Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache stack builder delegate which provides IServiceProvider #207

Closed
ErikPilsits-RJW opened this issue Jun 30, 2022 · 6 comments
Closed
Labels
enhancement New feature or request minor

Comments

@ErikPilsits-RJW
Copy link

My current scenario is that I have a class which provides my Redis connection, and which I register as a singleton in DI. I share this connection with other services in my project via DI. The new fluent cache stack builder doesn't provide me a way to resolve my connection provider when building the cache stack.

I also don't need the context in my cache stacks, but I do need to register multiple cache stacks. So below, I've subclassed CacheStack to register within DI.

I'm having to do something like this currently.

services.AddSingleton(sp =>
{
    var redisProvider = sp.GetRequiredService<IRedisConnectionProvider>();

    return new MainCacheStack(
        new ICacheLayer[]
        {
            new MemoryCacheLayer(),
            new RedisCacheLayer(redisProvider.RedisConnection, new RedisCacheLayerOptions(ProtobufCacheSerializer.Instance, redisOptions.Database))
        },
        new ICacheExtension[]
        {
            new AutoCleanupExtension(TimeSpan.FromMinutes(5)),
            new RedisLockExtension(redisProvider.RedisConnection,
                new RedisLockOptions(TimeSpan.FromMinutes(1), "CacheTower:CacheLock", "Lock:{0}", redisOptions.LockDatabase,
                    LockCheckStrategy.WithSpinLock(TimeSpan.FromMilliseconds(100)))),
            new RedisRemoteEvictionExtension(redisProvider.RedisConnection)
        });
});
@ErikPilsits-RJW ErikPilsits-RJW added the enhancement New feature or request label Jun 30, 2022
@Turnerj
Copy link
Member

Turnerj commented Jul 2, 2022

Hey @ErikPilsits-RJW - thanks for raising this issue!

The new fluent cache stack builder doesn't provide me a way to resolve my connection provider when building the cache stack.

Yeah, that's an oversight on my part. I'll likely add a variation of the builder that injects the service provider in so you can resolve your types directly. The reason it doesn't now is that the cache layers and extensions are built before registering the CacheStack within the IServiceCollection.

I also don't need the context in my cache stacks, but I do need to register multiple cache stacks. So below, I've subclassed CacheStack to register within DI.

Interesting. Cache Tower was definitely built with the idea that not everyone needs to provide a context (though it is a must if you want background refreshing to work reliably due to object lifetimes etc). With having multiple cache stacks, yeah, that is a bit of a problem. I'm glad you've got a solution that works for you at the moment though I get it isn't ideal.

One though I had was maybe approaching it the same way you can have ILogger<MyType> and that injects a specialized version of the dependency into your code. So for a CacheStack, I can't use ICacheStack<T> as that is already used for context support (not exactly a breaking change I'm comfortable making) but something like IScopedCacheStack<T> where T is your type. Then in your code, you'd have IScopedCacheStack<MyType> to inject your specific variation of it into your class. This would avoid you needing to subclass CacheStack yourself.

What's your thoughts on that?

@Turnerj
Copy link
Member

Turnerj commented Jul 2, 2022

I'm also not 100% on the it being called IScopedCacheStack as that may imply other things like DI lifetimes so if you have suggestions, I'd be happy to hear them!

@Turnerj Turnerj added the minor label Jul 2, 2022
@ErikPilsits-RJW
Copy link
Author

I would have been comfortable using ICacheStack<T> with different context types for my different layers if I could have resolved dependencies. In fact I was going to use it in my solution, however I wasn't able to directly create a CacheStack<T> due to the protection level of ServiceProviderContextActivator. I didn't feel like duplicating that code locally.

My initial thought for supporting multiple stacks was something akin to IHttpContextFactory where you could register a named stack and retrieve it through a factory class.

The background refreshes are an interesting situation (having re-read it now). Thinking on my usage I believe I'm ok without using the context (project has been running for almost 2 years without it). I don't refresh directly from any disposables that might be disposed externally, it's all api based data fetches.

@Turnerj
Copy link
Member

Turnerj commented Jul 3, 2022

Hey @ErikPilsits-RJW - I'd love for you to have a look at #208 to see whether that would cover your situation well. It adds support for named ICacheStack and ICacheStack<TContext> amongst some other minor DI improvements. It follows the pattern that IHttpClientFactory uses but with ICacheStackAccessor and ICacheStackAccessor<TContext>.

(I chose the suffix "Accessor" as while it does "create" like a factory, it doesn't create every call as the cache stacks themselves are singleton - I may still change this before merging - if you have any thoughts with this, would love to hear it)

@ErikPilsits-RJW
Copy link
Author

On first look it's exactly what I need. I guess it's actually more like IOptionsSnapshot<T>.

One thought though. Your current implementation creates new provider, lookup, and accessor singletons for every type of ICacheStack<TContext>. I don't think that's strictly necessary. The provider and lookup could hold object which gets typed in the accessor.

@Turnerj
Copy link
Member

Turnerj commented Jul 10, 2022

This is fixed via #208 with both new overloads for AddCacheStack that provide IServiceProvider within the builder as well as more overloads that provide named dependency support. This will be released in a new version of Cache Tower soon. :)

@Turnerj Turnerj closed this as completed Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor
Projects
None yet
Development

No branches or pull requests

2 participants