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

Implicit assumption in DI behavior when injecting IServiceProvider into scoped or transient services #63225

Closed
lord-executor opened this issue Dec 30, 2021 · 3 comments

Comments

@lord-executor
Copy link
Contributor

I came across this issue after a couple of hours of debugging an issue with the Blazor (.NET 6) service configuration. There seems to be an implicit assumption that is currently not covered by any of the existing specification / compliance tests in DependencyInjectionSpecificationTests: When an instance of IServiceProvider is requested from a scoped (non-root) service provider or a non-singleton instance that is created from such a service provider depends on an IServiceProvider then that scoped service provider is resolved and not the root service provider.

There is already a test for singletons that is DependencyInjectionSpecificationTests.SingletonServiceCanBeResolvedFromScope which works differently and always expects the "root service provider" to be injected.

As far as I can tell, the expected behavior is roughly this:

  • If an IServiceProvider is requested, then that should normally resolve to the service provider instance on which that request is made, meaning that the scope of the injected service provider is preserved, unless...
  • If an IServiceProvider is injected into a singleton service, then the root service provider should be resolved because singletons are always "attached" to the root service provider to control their lifetime and injecting a scoped service provider instance would be pretty dangerous.

To check if I was the only person that was unaware of this implicit assumption, I added a new test to cover that and got the following results: Of the 8 currently included DI frameworks, 4 conform to this implicit assumptions and 4 fail the new tests.

Grace => FAIL
Autofac => FAIL
LightInject => FAIL
StashBox => FAIL
DryIoc => OK
StructureMap => OK
Lamar => OK
Unity => OK

At this point, I can almost guarantee that any DI container that doesn't satisfy this implicit assumption will run into problems when running even the basic Blazor project template. Particularly if that container does cover the singleton special case which seems to be true for all of the failing containers. Since that will end up in the same case I had issues with my Ninject integration where services are incorrectly resolved using the root service provider instead of the scoped service provider.

I can give more details on this exact Blazor scenario if needed - it mostly revolves around the Microsoft.AspNetCore.Components.NavigationManager.

Of course, like all critical assumptions of Microsoft.Extensions.DependencyInjection, this one should also be covered by a compliance test and I will create a PR for that and update the tests for the currently failing external DI containers to exclude this test case using the SkippableDependencyInjectionSpecificationTests mechanism.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-DependencyInjection untriaged New issue has not been triaged by the area owner labels Dec 30, 2021
@ghost
Copy link

ghost commented Dec 30, 2021

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

I came across this issue after a couple of hours of debugging an issue with the Blazor (.NET 6) service configuration. There seems to be an implicit assumption that is currently not covered by any of the existing specification / compliance tests in DependencyInjectionSpecificationTests: When an instance of IServiceProvider is requested from a scoped (non-root) service provider or a non-singleton instance that is created from such a service provider depends on an IServiceProvider then that scoped service provider is resolved and not the root service provider.

There is already a test for singletons that is DependencyInjectionSpecificationTests.SingletonServiceCanBeResolvedFromScope which works differently and always expects the "root service provider" to be injected.

As far as I can tell, the expected behavior is roughly this:

  • If an IServiceProvider is requested, then that should normally resolve to the service provider instance on which that request is made, meaning that the scope of the injected service provider is preserved, unless...
  • If an IServiceProvider is injected into a singleton service, then the root service provider should be resolved because singletons are always "attached" to the root service provider to control their lifetime and injecting a scoped service provider instance would be pretty dangerous.

To check if I was the only person that was unaware of this implicit assumption, I added a new test to cover that and got the following results: Of the 8 currently included DI frameworks, 4 conform to this implicit assumptions and 4 fail the new tests.

Grace => FAIL
Autofac => FAIL
LightInject => FAIL
StashBox => FAIL
DryIoc => OK
StructureMap => OK
Lamar => OK
Unity => OK

At this point, I can almost guarantee that any DI container that doesn't satisfy this implicit assumption will run into problems when running even the basic Blazor project template. Particularly if that container does cover the singleton special case which seems to be true for all of the failing containers. Since that will end up in the same case I had issues with my Ninject integration where services are incorrectly resolved using the root service provider instead of the scoped service provider.

I can give more details on this exact Blazor scenario if needed - it mostly revolves around the Microsoft.AspNetCore.Components.NavigationManager.

Of course, like all critical assumptions of Microsoft.Extensions.DependencyInjection, this one should also be covered by a compliance test and I will create a PR for that and update the tests for the currently failing external DI containers to exclude this test case using the SkippableDependencyInjectionSpecificationTests mechanism.

Author: lord-executor
Assignees: -
Labels:

untriaged, area-Extensions-DependencyInjection

Milestone: -

@lord-executor
Copy link
Contributor Author

Also interesting: Unity seems to have the exact opposite issue since it already excludes the SingletonServiceCanBeResolvedFromScope test wich covers the singleton scenario but runs fine against my new test.

@eerhardt eerhardt added this to the 7.0.0 milestone Jan 3, 2022
@eerhardt eerhardt removed the untriaged New issue has not been triaged by the area owner label Jan 3, 2022
lord-executor added a commit to lord-executor/runtime that referenced this issue Jan 31, 2022
eerhardt pushed a commit to lord-executor/runtime that referenced this issue Feb 7, 2022
- Refactored existing singleton test to avoid testing service provider reference equality
@eerhardt
Copy link
Member

Fixed by #64558. Closing.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants