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.

Sign up for GitHub

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

Jump to bottom

Explicit test for injecting scoped IServiceProvider into scoped and transient services (#63225) #63226

Closed
wants to merge 1 commit into from
Closed

Explicit test for injecting scoped IServiceProvider into scoped and transient services (#63225) #63226

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions ...ions.DependencyInjection.Specification.Tests/src/DependencyInjectionSpecificationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,31 +168,45 @@ public void SingletonServiceCanBeResolvedFromScope()
{
// Arrange
var collection = new TestServiceCollection();
// When resolved from the root service provider / root scope, the scoped IFakeService effectively
// also becomes a singleton.
collection.AddScoped<IFakeService, FakeService>();
collection.AddSingleton<ClassWithServiceProvider>();
var provider = CreateServiceProvider(collection);

// Act
IServiceProvider scopedSp1 = null;
IServiceProvider scopedSp2 = null;
ClassWithServiceProvider instance1 = null;
ClassWithServiceProvider instance2 = null;
IFakeService fakeServiceFromScope1 = null;
IFakeService scopedFakeServiceFromScope1 = null;
IFakeService fakeServiceFromScope2 = null;
IFakeService scopedFakeServiceFromScope2 = null;

using (var scope1 = provider.CreateScope())
{
scopedSp1 = scope1.ServiceProvider;
instance1 = scope1.ServiceProvider.GetRequiredService<ClassWithServiceProvider>();
scopedFakeServiceFromScope1 = scope1.ServiceProvider.GetRequiredService<IFakeService>();
var serviceWithProvider = scope1.ServiceProvider.GetRequiredService<ClassWithServiceProvider>();
// resolved through singleton, the fake service should be scoped to root scope
fakeServiceFromScope1 = serviceWithProvider.ServiceProvider.GetRequiredService<IFakeService>();
}

using (var scope2 = provider.CreateScope())
{
scopedSp2 = scope2.ServiceProvider;
instance2 = scope2.ServiceProvider.GetRequiredService<ClassWithServiceProvider>();
scopedFakeServiceFromScope2 = scope2.ServiceProvider.GetRequiredService<IFakeService>();
var serviceWithProvider = scope2.ServiceProvider.GetRequiredService<ClassWithServiceProvider>();
// resolved through singleton, the fake service should be scoped to root scope
fakeServiceFromScope2 = serviceWithProvider.ServiceProvider.GetRequiredService<IFakeService>();
}

IFakeService fakeServiceFromRootScope = provider.GetRequiredService<IFakeService>();

// Assert
Assert.Same(instance1.ServiceProvider, instance2.ServiceProvider);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is breaking the original intention of this test. See dotnet/extensions#2236 and dotnet/extensions#1301 for more information on why this test was written this way.

Copy link
Contributor Author

@lord-executor lord-executor Jan 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I don't think so. The fact that the instances are the same is not strictly necessary for #1301. All that is really needed is that the two providers are bound to the same (root) scope which the new version is validating with

Assert.Same(fakeServiceFromScope1, fakeServiceFromScope2);

In fact, if you add the very simple test

        [Fact]
        public void RootProviderEquality()
        {
            // Arrange
            var collection = new TestServiceCollection();
            collection.AddScoped<IFakeService, FakeService>();
            collection.Add(new ServiceDescriptor(typeof(ClassWithServiceProvider), typeof(ClassWithServiceProvider), ServiceLifetime.Scoped));
            var provider = CreateServiceProvider(collection);
            var sameProvider = provider.GetRequiredService<IServiceProvider>();
            var sameProvider2 = provider.GetRequiredService<IServiceProvider>();

            // Assert
            Assert.Same(sameProvider, sameProvider2);
            Assert.Same(provider, sameProvider);
        }

Then that fails for Grace, LightInject, Autofac and StashBox because they do not actually ensure that the service provider is the same instance. They do however ensure that the instances are tied to the same scope which is what these changes here are testing. Ensuring reference equality of the provider instance is sufficient, but not necessary. As long as the scope is the right one, the problem in #1301 should be solved.

And I did a similar test for scopes which generates similarly entertaining failures with the same set of containers and additionally DryIoc and Lamar also fail that one.

        [Fact]
        public void ScopedProviderEquality()
        {
            // Arrange
            var collection = new TestServiceCollection();
            collection.AddScoped<IFakeService, FakeService>();
            collection.Add(new ServiceDescriptor(typeof(ClassWithServiceProvider), typeof(ClassWithServiceProvider), ServiceLifetime.Scoped));
            var provider = CreateServiceProvider(collection);

            IServiceProvider scopeProvider = null;
            IServiceProvider sameProvider = null;
            IServiceProvider sameProvider2 = null;

            using (var scope = provider.CreateScope())
            {
                scopeProvider = scope.ServiceProvider;
                sameProvider = provider.GetRequiredService<IServiceProvider>();
                sameProvider2 = provider.GetRequiredService<IServiceProvider>();
            }

            // Assert
            Assert.Same(scopeProvider, sameProvider);
            Assert.Same(sameProvider, sameProvider2);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I actually enable that singleton test for Unity (where it is currently still disabled), then it does still fail that test, but in an "interesting" way:

Failed Microsoft.Extensions.DependencyInjection.Specification.UnityDependencyInjectionSpecificationTests.SingletonServiceCanBeResolvedFromScope [23 ms]
  Error Message:
   System.InvalidOperationException : No service for type 'Microsoft.Extensions.DependencyInjection.Specification.Fakes.IFakeService' has been registered.
  Stack Trace:
     at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType) in /workspaces/runtime/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceProviderServiceExtensions.cs:line 58
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider) in /workspaces/runtime/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceProviderServiceExtensions.cs:line 78
   at Microsoft.Extensions.DependencyInjection.Specification.DependencyInjectionSpecificationTests.SingletonServiceCanBeResolvedFromScope() in /workspaces/runtime/src/libraries/Microsoft.Extensions.DependencyInjection.Specification.Tests/src/DependencyInjectionSpecificationTests.cs:line 196

Meaning that when it tries to resolve the fake service in the second scope, it can't find a registration for IFakeService. This is admittedly not what I was expecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear. I'll of course revert the changes to this test if you prefer, but I do think that the reference equality testing on the service provider is unnecessary and misleading as demonstrated by my two simple demo tests above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as demonstrated by my two simple demo tests above.

This test is for Singleton services, and both those demo tests use Scoped services.

For Singleton services, I believe the intention is that the ServiceProvider they get is the root provider, and thus they should be the same.

@davidfowl - you originally wrote the test. Do you have any thoughts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this test is for Singleton services. And the ClassWithServiceProvider is still registered as a singleton and thus should be injected with the root provider. The scoped IFakeService is just my "canary in the coal mine" for figuring out if the singleton was actually injected with the root scope. It still tests the same scenario, just in a different way.

  • scopedFakeServiceFromScope1 is resolved from the scope's service provider => that should be a fresh instance bound to scope1
  • fakeServiceFromScope1 is resolved from the service provider that was injected into the singleton which should be the root scope => that should give me a different instance to scopedFakeServiceFromScope1
  • then we play the same game with scope2 and compare results based on where we expect the same or different instances

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw. the reason for the weird failure mode of Unity in the Singleton scenario has actually already been explained by @davidfowl in #extensions/1301 I just didn't read his explanation carefully enough.

Even though Foo is a singleton, the service provider injected is from the first scope it's resolved from. That's a recipe for disaster.

Unity fails for the same reason it failed before, because it injects the scope1 service provider into the singleton. That scope is then disposed and in the code for scope2 it is trying to resolve a service through this already disposed scope1 service provider which fails because it can no longer find the IFakeService registration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me take a deeper look at this today. On second glance I can see why my test would make other containers fail. The service provider instance isn't as important as the lifetime of the objects that come out of it.

Assert.NotSame(instance1.ServiceProvider, scopedSp1);
Assert.NotSame(instance2.ServiceProvider, scopedSp2);
// resolved through singleton should be the same and same with resolved from root scope
Assert.Same(fakeServiceFromScope1, fakeServiceFromScope2);
Assert.Same(fakeServiceFromRootScope, fakeServiceFromScope1);
Assert.Same(fakeServiceFromRootScope, fakeServiceFromScope2);
// resolved through scope should be different from corresponding scoped instance
Assert.NotSame(fakeServiceFromScope1, scopedFakeServiceFromScope1);
Assert.NotSame(fakeServiceFromScope2, scopedFakeServiceFromScope2);
Assert.NotSame(scopedFakeServiceFromScope1, scopedFakeServiceFromScope2);
}

[Fact]
Expand Down