pamidur / aspect-injector

AOP framework for .NET (c#, vb, etc)
Apache License 2.0
745 stars 112 forks source link

Caching doesn't work with scoped DI instances #144

Closed MaciejWanat closed 3 years ago

MaciejWanat commented 3 years ago

I've been recently looking for a library for simple, effective caching using attributes, and your library with cache extension does pretty much exactly what I've been looking for.

The problem I've encountered is that I've used it with pretty standard ASP.NET application, with multiple sercives registered as scoped. In this quite standard scenario caching doesn't work, because cache key in your implementation is created basing on instance - and the instance is different with each request (https://github.com/pamidur/aspect-injector/blob/master/samples/src/Cache/CacheAspect.cs#L83).

Would you consider addresing that problem? The solution I've used is to simply base on method name + arguments as a key. I had to adjust it for my project myself, so I could give a PR for you if you accept external PRs.

pamidur commented 3 years ago

Hi @MaciejWanat , your suggestion does sound good! What about using type instead of instance? it'll have about the same entropy but will still work with different instances. I'd happily accept your PR!

Thank you for your interest in this project

MaciejWanat commented 3 years ago

Great! I will prepare a PR soon.

pamidur commented 3 years ago

I was thinking we could ither move key calculation to attribute or just add additional parameter to indicate if cache should be per instance:

    [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
    public class MemoryCacheAttribute : CacheAttribute
    {
        private static readonly MemoryCache _cache = new MemoryCache("aspect_builtin_memory_cache");
        private readonly uint _seconds;

        public bool PerIntsanceCache {get;set;} = true;

        public MemoryCacheAttribute(uint seconds)
        {
            _seconds = seconds;
        }

        public override ObjectCache Cache => _cache;
        public override CacheItemPolicy Policy => new CacheItemPolicy() { AbsoluteExpiration = DateTimeOffset.UtcNow.AddSeconds(_seconds) };
    }

//the use it like this
[MemoryCache(3000, PerIntsanceCache = false)]

Other option is to move key calculation to the attribute:

    [AttributeUsage(AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
    [Injection(typeof(CacheAspect), Inherited = true)]
    public abstract class CacheAttribute : Attribute
    {
        public abstract ObjectCache Cache { get; }

        public abstract CacheItemPolicy Policy { get; }

        public virtual GetKey(type, instance, method, args) { .. }
    }