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

Resolving last registration from multiple default services #97

Closed
CoskunSunali opened this issue Mar 25, 2019 · 25 comments
Closed

Resolving last registration from multiple default services #97

CoskunSunali opened this issue Mar 25, 2019 · 25 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@CoskunSunali
Copy link

Do you have any opinion on why the following container would return the first registration of a service when multiple registrations exist? That is contrary to what IServiceProvider does so I am a bit confused.

var container = new Container(rules => rules
    .With(FactoryMethod.ConstructorWithResolvableArguments)
    .WithFactorySelector(Rules.SelectLastRegisteredFactory())
    .WithTrackingDisposableTransients()
    .WithCaptureContainerDisposeStackTrace());

container.Register<IService, A>(reuse: Reuse.Singleton);
container.Register<IService, B>(reuse: Reuse.Transient);

var instance = container.Resolve<IService>(); // returns A

I am almost certain that you have implemented an option to change this behavior but I cannot find it.

Thanks for your support in advance.

@CoskunSunali
Copy link
Author

CoskunSunali commented Mar 25, 2019

Investigating further discloses that if I register both services as singletons, it works as I expect it. However, if the first registration is singleton and the next one is transient, it resolves using the singleton registration.

You may ask why would I register the same service using different lifetimes. It is not me actually, it is IdentityServer4 but what they do makes sense to me too.

Their AddIdentityServer method adds a default in-memory IPersistedGrantStore service which is singleton. They also have a method called AddPersistedGrantStore where we can set the store implementation type and they register it as transient this time. This works fine with IServiceProvider but fails as soon as I replace the provider with DryIoc.

I would still like to know if there is a way to make DryIoc resolve to the last registration no matter what lifetime is used during registrations.

@CoskunSunali
Copy link
Author

Unfortunately I am not able to interfere with scopes used internally within IdentityServer 4.

I am also unable to change the way the services are registered internally. They only call AddSingleton for the default registration and AddTransient for the custom one.

@dadhi
Copy link
Owner

dadhi commented Mar 25, 2019

I mean, turn Off the implicit scoped service selection:

var container = new Container(rules =>
            rules.WithoutImplicitCheckForReuseMatchingScope());

@CoskunSunali
Copy link
Author

I see. Thanks for the lead.

Doesn't that mean I won't be able to take advantage of Reuse.ScopedTo in the other parts of the application (shared container) which I would really like to be able to because it is a nice feature of DryIoc?

@dadhi
Copy link
Owner

dadhi commented Mar 25, 2019

No, it does not mean that, cause ScopedTo requires a specific scope target.

@CoskunSunali
Copy link
Author

I mean, based on the documentation's example:

        var container = new Container(rules =>
            rules.WithoutImplicitCheckForReuseMatchingScope());

        container.Register<I, A>(Reuse.ScopedTo("a"));
        container.Register<I, B>(Reuse.ScopedTo("b"));

        using (var scopeB = container.OpenScope("b"))
        {
            // Throws an exception because of the multiple `I` registrations found
            Assert.Throws<ContainerException>(() => scopeB.Resolve<I>());
        }

A scope named b is opened but resolving the service I throws because there is also one registered to scope a. We have nothing to do with scope a and we are explicitly working in scope b. It might be my understanding or ignorance but it seems that ScopedTo doesn't have any use case once you call WithoutImplicitCheckForReuseMatchingScope because it throws even though I registrations are registered to different scopes.

I think the default behavior of ScopedTo, the one without having WithoutImplicitCheckForReuseMatchingScope called and the one that resolves I to type of B seems the correct approach. However, just to get the "resolve to the last one registered" work I am being have to disable the correct approach of named-scope registrations.

Sorry I am struggling to understand.

@dadhi
Copy link
Owner

dadhi commented Mar 25, 2019

Hmm, OK. I need to re-check. Maybe there is an issue here.

@CoskunSunali
Copy link
Author

Thanks in advance!

@dadhi
Copy link
Owner

dadhi commented Mar 25, 2019

Yeah :( from this perspective it is a problem.
Essentially it boils down to the place in pipeline where SelectLastFactory is inserted. Will think how to fix it in the least breaking manner.

@CoskunSunali
Copy link
Author

Glad we agree that it is a problem from some perspective.

Please let me know if you want me to test it when you have a solution.

@SeriousM
Copy link
Contributor

SeriousM commented Mar 26, 2019

Unfortunately I am not able to interfere with scopes used internally within IdentityServer 4.

I am also unable to change the way the services are registered internally. They only call AddSingleton for the default registration and AddTransient for the custom one.

I had a similar issue: #76, AliBazzi/IdentityServer4.Contrib.RedisStore#14
Does this help you in any way?

@dadhi
Copy link
Owner

dadhi commented Mar 26, 2019

@SeriousM Thanks for more info.
Actually, it should not be a problem with the latest DryIoc.Microsoft.DependencyInjection, because it is already uses option WithoutImplicitCheckForReuseMatchingScope.

@SeriousM
Copy link
Contributor

you're welcome

@CoskunSunali
Copy link
Author

@SeriousM I actually don't use the DryIoc.Microsoft.DependencyInjection package. I have my own adapter, due to other reasons that doesn't concern DryIoc. Apparently when you use the DI adapter, things should work, as @dadhi said, because it already uses WithoutImplicitCheckForReuseMatchingScope but in my case, we are trying to avoid using that option because we really like how ScopedTo services work. ScopedTo is a brilliant feature of DryIoc that I don't want to give up on :(

@dadhi
Copy link
Owner

dadhi commented Mar 26, 2019

Fix is on the way :)

@CoskunSunali
Copy link
Author

🕺

@CoskunSunali
Copy link
Author

@SeriousM I forgot to thank you for commenting and sharing what information you have. Thank you too.

@dadhi dadhi closed this as completed in 5ebcd5a Mar 28, 2019
dadhi added a commit that referenced this issue Mar 28, 2019
…ndencyInjectionRules and from DryIocAdapter as it is not needed after fixing the #97
@dadhi dadhi self-assigned this Mar 28, 2019
@dadhi dadhi added the bug Something isn't working label Mar 28, 2019
@dadhi dadhi added this to the v4.0.1 milestone Mar 28, 2019
@CoskunSunali
Copy link
Author

So, does your commit mean that when using “select last registered”, the container will work, by default, the same way Microsoft’s service provider works?

@dadhi
Copy link
Owner

dadhi commented Mar 28, 2019

Yes, if you are using Rules.SelectLastRegisteredFactory(), it will override implicit scope based selection, if you have multiple candidates. Like in case of Singleton and Transient.
If you have a single candidate e.g. ScopedTo, then it will be selected.

@CoskunSunali
Copy link
Author

Great work! Thanks for your quick reaction.

Too much if I ask the release date for 4.0.1?

@dadhi
Copy link
Owner

dadhi commented Mar 28, 2019

... in case you have Singleton or Transient and ScopedTo, and all of them can be applied, then the last registered will be selected.

@dadhi
Copy link
Owner

dadhi commented Mar 28, 2019

Will release Today or Tomorrow's morning.

@dadhi
Copy link
Owner

dadhi commented Mar 28, 2019

It is out.

@CoskunSunali
Copy link
Author

Great! I’ll go ahead and test it ASAP.

Thanks a lot for your efforts!

Leszek-Kowalski pushed a commit to Leszek-Kowalski/DryIoc that referenced this issue Oct 11, 2019
…vices

changed: using the factory selection rule when multiple services are matching the reuse
less allocation of rule factory selection path
added to the docs the example with SelectLastRegisteredFactory and Singleton and Transient service
Leszek-Kowalski pushed a commit to Leszek-Kowalski/DryIoc that referenced this issue Oct 11, 2019
…ndencyInjectionRules and from DryIocAdapter as it is not needed after fixing the dadhi#97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants