-
Notifications
You must be signed in to change notification settings - Fork 117
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
[FEATURE] 🔑 Add DI Keyed Services support #238
Comments
Based on my understanding the plan here is to make functionally equivalent the usage of the named caches feature of Fusion Cache with the keyed services feature of Microsoft DI. Based on your explanation, you want that any code using a named cache registration should also be able to resolve the named cache instance by using the [FromKeyedServices attribute]. This basically means, as you said, to also register a keyed service each time a named cache is registered (unfortunately, doing that implicitly has all the problems you very well explained). I have a few questions, to better understand your idea and the reasons behind it. Here are my questions:
Maybe answering these questions could help in deciding whether or not to implement this feature. By the way, I think that a very big advantage of the proposed opt-in approach is that by doing so you are sure not to break any existing code. Basically if the client code doesn't call |
Ideally yes, I think this would've been great.
I uno-reverse the question: why should I wait?
No: apart from the fact that having "named resolving to keyed resolving to named etc" would probably create an infinite loop, but the way to register FusionCache is via the
Exactly this.
I don't see a need to deprecate named caches, so I'd like to keep supporting both (if you see a reason why let me know, I'm curious). Also, the named caches approach is more powerful than the keyed one, since it allows programmatic access instead of declarative: basically the keyed approach is the simple and declarative version of the more flexible named approach.
Yep, exactly.
Got it, thanks for sharing! |
I would like to offer another perspective. Basically your initial goal was that this keyed service approach will just work. Having to enable this feature explicitly is contrary to that goal as you have to explicitly search for such a method, which is unexpected. Setting aside the problem of other DI implementations for a second, with the latest Microsoft DI, enabling keyed services by default should not be a breaking change as nothing will change unless you specifically asked for a keyed service. Agreed? That leaves the issue of old Microsoft DI implementations or alternative DIs, which (for better or for worst) is the minority by now. In these DIs trying to register the services will cause an exception to be thrown. I would suggest to catch and wrap this exception in a custom exception that explains the problem and gives a clear instruction on how to opt out of this behavior. As DI registration happens at startup, it is impossible to miss this change after updating Fusion cache, even if you missed the change in the documentation. Of course it's a small inconvenience for a small number of users, but so is requiring the majority of users to opt-in. I think it's also fair to assume that the number of users using DIs that do not support keyed services will decrease over time, so I think it's better to use a solution that is prepared for this future. |
Hi @aKzenT , thanks for chipping in!
Agree.
Not really: I mean people registering other keyed services manually know that they have to call AddKeyedEtc, and here they would call
Nope, building the
Update (regarding the other points you've made)
Eh, that was the first thought I had 😅, but... the exception is not thrown when adding the keyed service, but later on when building the service provider.
I thought about it (eg: "just let it explode in a potentially low number of cases"), but as I said I can't control it, throw/log some useful message or else, so that it would become a surprise and people would need to try to understand what is going on.
With this in mind and all things considered, I'm feeling it's the best approach. Having said all of this, what do you think? I'm still open for new point of views. |
Based on my understanding, your initial idea was to add an implicit support to keyed service resolution for named caches because, in a sense, it should be the natural expectation of a user: from a certain point of view, it feels natural being able to resolve a named cache as a keyed service. Reflecting on your answer to my previous post, it seems to me that these features are just "weakly" connected: they are there to solve different needs and, as you pointed out, named caches are a more flexible approach since they are tied to the usage of compile time constants (which, instead, must be used with keyed services because of the If you put yourself in the user shoes, an implicit support to keyed service resolution to named caches comes at the cost of side effects:
To summarize, regardless of the runtime exception, the implicit support for keyed service with named caches is a breaking change to the library: the runtime exception simply makes it worse. Final point. Since you pointed out that the exception is thrown when the service provider is built from the service collection, there is no way for you to intercept this problem. Based on my understanding, the service provider is built when the host is built, usually inside the For all of these reasons, I think that designing this feature (keyed services support for named caches) as an opt-in feature is even better than your initial idea of an out of the box implicit support. This way, the client code is in full control and can opt-in by explicitly calling |
Hi @EnricoMassone and thanks for taking the time to answer in such a complete way!
I don't know if I would call them "weakly connected", maybe more like "the features of keyed services are a subset of the ones of named caches" but yeah, I got your point and it makes sense.
I suppose you meant "they are NOT tied to the usage of compile time constants"?
Yes, technically.
Agree.
Exactly.
99% agree, and this is how it is currently implemented in my local branch, and it is working well 🙂 Thanks again, much appreciated! |
Good afternoon @jodydonetti you are welcome.
Yes, there was a typo here. Actually what I meant is that named caches are not tied to the usage of compile time constants. Of course, let's also wait for other people point of view. Designing public APIs is never easy. Anyway, joining this kind of discussions is a great opportunity to learn something new and to experience the challenges of designing a public library like this. Keep on with the great job on Fusion Cache! |
Hi all, I just updated the original proposal, look for the "Update 2024-05-10" part at the bottom. It's not about registering stuff but about adding support for resolving keyed registered services for things like the backplane, the distributef cache, etc. Opinions? |
Read the whole discussion, and I'm not sure if I missed this point, but the nice thing about AsKeyedServices is also that in a distant future, where all DI providers support this feature, you can simply make the method obsolete. Early adopters will hopefully keep reading changelogs, and the silent majority will be none the wiser. |
Based on my understanding, with the current implementation of methods For instance, let's suppose we have:
in this case, an instance of What happens, in the current version of Fusion Cache, if a user registers multiple implementations of the
Based on my understanding, in this case the last registration wins, so the resolved instance of I'm asking this just to understand the actual use case to support keyed services for things like backplanes. If I get it right, with the current version there is not the possibility of registering different backplanes and then pick one of them to be used with the cache: as explained above, the last one registered wins and it is the one actually used. Is the use case behind this new feature to let a user define multiple backplane implementations, give them a name, and use them with different caches (by picking the chosen one by name) ? |
Correct.
Yes, I'm not 100% sure since it's not that common, but I think so. Anyway, it's basically a way to say "use THE backplane is that has been registered via DI".
The use case, in general, is to add support for keyed services everywhere it can make sense: at first it was about FusionCache registering itself to be later resolved by user code via A concrete example I can think about for this scenario is registering 2 different
Not directly, but it's possible to do this: services.AddFusionCache()
.WithBackplane(sp =>
{
var backplanres = sp.GetServices<IFusionCacheBackplane>();
// TODO: USE A LOGIC HERE...
return backplanres.First();
});
Kinda, we can say this.
Yes. |
Ok, now it makes more sense to me. Thanks ! In general, this seems to be a good idea. My only point is about this overload:
Is this really needed ? Probably your rationale is that in many cases the user will register a named backplane having the very same name of the cache. Correct ?
In this case you don't have a cache name, at least you don't have an explicit one (I don't know if there is an implicit default cache name in this case). |
Correct.
Any cache has a name underneath, including the default one: for that, the name is Anyway I see your point, and in fact I'm still not sure about the parameterless version: on one hand it makes sense to specify the name just once (with |
Hi all, I just released v1.2.0-preview1 🥳 |
Hi @EnricoMassone , did you happen to take a look at or play with preview1? |
Good evening, I just tried it in a little toy project and it seems to work fine. |
Thanks @EnricoMassone ! |
Hi all, FusionCache v1.2.0 is out 🥳 |
Intro
Sometimes while working with DI in .NET we may need to register multiple services of the same type, but configured differently.
The classic example is with multiple HTTP clients configured differently and referenced via a name: the .NET team solved this with HTTP named clients by using a custom approach for that scenario which included the introduction of the
services.AddHttpClient("name", ...)
ext method to setup one,IHttpClientFactory
as the dependency to take and finally theIHttpClientFactory.CreateClient("name")
method to get one.FusionCache had the same need, in the form of Named Caches: since the approach was sound, it worked well and the .NET community was already familiar with it, I used it as a blueprint for how to do it in FusionCache, too.
So if with HTTP client we can do this:
AddHttpClient("name", ...)
IHttpClientFactory
CreateClient("name")
HttpClient
with FusionCache we can do this:
AddFusionCache("name", ...)
IFusionCacheProvider
GetCache("name")
IFusionCache
All went well and the community response has been great.
Enter Keyed Services
Since .NET 8 we now have native support for multiple services of the same type, identified by different names, thanks to the addition of so called keyed services.
The idea is basically that we can now register services not only by type but also by specifying the name, like this:
and later, to resolve it, simply mark a constructor parameter or web action with the
FromKeyedServices
attribute, like this:Note that at least currently (during the .NET 9 wave) the HTTP named clients story still seems not to have been updated to support keyed services yet (see here), nonetheless I was thinking about looking to add support to FusionCache.
So, here we are.
Proposed Design
The idea as always is that, as much as possible, things should just work.
This means that users already registering named caches via
AddFusionCache("name")
should be able to then resolve them not just withIFusionCacheProvider.GetCache("name")
but also via[FromKeyedServices("name")] IFusionCache cache
, all transparently, meaning with only one registration, both ways of resolving it would be available, all for free.But how?
Easy, inside the implementation of the
AddFusionCache("name")
method I would register it both "normally" (to be later resolved viaIFusionCacheProvider
) and also as a keyed service, with a factory that just resolve the "normal" one and return it.Something like this:
Therefore I started implementing it, and... problems.
Problems
Here's how how it started, and it basically boils down to thiese points:
Microsoft.Extensions.DependencyInjection.Abstractions
>= 8 it's possible to callAddKeyedSingleton<TService>
and friendsBuildServiceProvider()
)Microsoft.Extensions.DependencyInjection
package (which depends onMicrosoft.Extensions.DependencyInjection.Abstractions
above), and from >= 8 it supports keyed servicesIt all boils down to the fact that it is not possible to detect if the container in use does support keyed services or not, therefore FusionCache cannot do anything automatically.
Some people suggested to use conditional compilation with something like
#if NET8_0_OR_GREATER
, but that doesn't work.First because that only checks the .NET runtime version, not the version of the
Microsoft.Extensions.DependencyInjection
package (that contains the standard DI container impl): they are not the same and we cannot infer one from the other, as can be seen here.Second because, as said, there may be different containers in use (like Lamar, etc) so the version of the standard Microsoft impl doesn't really tell use anything.
So basically, as of today, the design of the available abstractions in .NET seem to prevent a form of feature detection, therefore not allowing me to do this both well and in a transparent way 😞
(unless I'm missing something... and in that case please let me know!)
A Possible Solution
So, the core block is that it's not possible to do keyed registrations automatically because we want to avoid throwing random uncontrolled exceptions, right?
So, let's keep it manual! But wait, not with a different method altogether like
AddKeyedFusionCache("cache")
, that would ONLY register it as a keyed service forcing the user to choose one or the other, BUT with a very simple opt-in.Something like this:
In this way the decision will be in the hand of the user actually registering the services, and they will know for sure if they want a keyed service or not, right?
Opinions?
What do you all think? Looks good? Not? Why?
Please le me know, thanks!
Update 2024-05-10
While playing with the prototype, I noticed another point where keyed services may come into play: the
WithRegisteredXyz()
methods on the builder.Right now we can do this for example:
and it will look for an
IFusionCacheBackplane
service registered via DI.By introducing support for keyed services, a natural additiona would be something like:
or this:
Both will look for registered keyed services of type
IFusionCacheBackplane
: in the first case the keyed service name is directly provided ("foo"
) while in the second case the keyed service name is the name of the cache ("MyCache"
).I should add this type of support to all
WithRegisteredXyz()
andTryWithRegisteredXyz()
methods for the backplane, the distributed cache, the serializer, etc.Opinions?
The text was updated successfully, but these errors were encountered: