-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Explicit test for injecting scoped IServiceProvider into scoped and transient services (#63225) #63226
Explicit test for injecting scoped IServiceProvider into scoped and transient services (#63225) #63226
Conversation
Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection Issue DetailsThe new test is analogous to the existing I have also updated the tests of the 4 failing external DI containers (Grace, Autofac, LightInject, StashBox) to skip this new test in order to both, keep the tests green and serve as an indicator to the other maintainers for where they are not 100% compliant.
|
c6b3a1f
to
b375e42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are the 4 external DI containers failing? Are they all failing for the same reason?
cc @davidfowl - in case you have thoughts on the new test. I think it looks OK.
Good question. And I should have thought of that before. Currently, the errors look like this: Autofac
Grace
LightInject
StashBox
And I did some more "variations" on this test case on my machine which raises an additional question. The existing Comparing the service provider that is injected into the
then
Assumptions are hard 😂. Generally speaking, checking service providers with reference equality seems a bad idea since that does introduce the additional assumption that new instances of the service provider are only created when a new scope is created and that does not apply to all containers. In the existing
If I change the test to the sample code above AND if I limit the test to scoped binding of I'll go and experiment some more with some more elaborate test setups so that the test does not have to rely on service provider reference equality at all but instead only tests the actual service instantiation behavior when creating service instances through a "captured" service provider like in |
b375e42
to
dced533
Compare
OK, now we have version 2.0 of the new test. I replaced the old commit with the new one. @eerhardt & @davidfowl The "trick" for testing if the service provider is tied to the correct scope is simply to use it to instantiate a scoped service ( With this new implementation, only LightInject fails with
which means that LightInject is always using the root scope service provider since that is the only way that the scoped |
// Assert | ||
Assert.Same(instance1.ServiceProvider, instance2.ServiceProvider); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 toscope1
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 toscopedFakeServiceFromScope1
- then we play the same game with
scope2
and compare results based on where we expect the same or different instances
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new test looks fine. But I think the changes to the existing (singleton) test should get reviewed by @davidfowl, who is out until next week.
No worries. It's not like this is an urgent issue, so I'll just wait and be patient. |
@davidfowl thoughts here? I still think the existing test should remain as-is. It is a simpler way of testing the scenario, instead of the indirection being added here. |
@eerhardt : I wasn't quite sure about the protocol and I didn't want to cause too much confusion by just completely removing the code that the previous discussion is all about, so I just created a seperate PR #64558 which only contains the new test and leaves the existing one alone. If you like that one better, then I'll just close this one. |
- Refactored existing singleton test to avoid testing service provider reference equality
dced533
to
d3f9d86
Compare
I've merged #64558, so the new test is now added. All that remains here is the refactoring of the existing test. @davidfowl - thoughts? |
Thanks for the work here, @lord-executor. I'm going to close out this PR since I don't believe this test refactoring is adding value and is making it harder to understand what is being tested by adding the extra indirection. If the current test proves to be a problem with DI implementers, we can re-open it. |
The new test is analogous to the existing
SingletonServiceCanBeResolvedFromScope
but covers the more general (at least from my point of view) case of non-singleton injection.I have also updated the tests of the 4 failing external DI containers (Grace, Autofac, LightInject, StashBox) to skip this new test in order to both, keep the tests green and serve as an indicator to the other maintainers for where they are not 100% compliant.