A design issue with scoping and how it disposes transients without intent or graphing of lifestyles
Opened this issue · 10 comments
Hi
I am trying to implement a IServiceProvider for the v4 release of Castle Windsor. I invite you to consider this test:
[Fact]
public void DisposingScopeDisposesService()
{
// Arrange
var collection = new TestServiceCollection();
ServiceCollectionServiceExtensions.AddSingleton<IFakeSingletonService, FakeService>(collection);
ServiceCollectionServiceExtensions.AddScoped<IFakeScopedService, FakeService>(collection);
ServiceCollectionServiceExtensions.AddTransient<IFakeService, FakeService>(collection);
var provider = CreateServiceProvider(collection);
FakeService disposableService;
FakeService transient1;
FakeService transient2;
FakeService singleton;
// Act and Assert
var transient3 = Assert.IsType<FakeService>(ServiceProviderServiceExtensions.GetService<IFakeService>(provider));
using (var scope = ServiceProviderServiceExtensions.CreateScope(provider))
{
disposableService = (FakeService) scope.ServiceProvider.GetService<IFakeScopedService>();
transient1 = (FakeService) scope.ServiceProvider.GetService<IFakeService>();
transient2 = (FakeService) scope.ServiceProvider.GetService<IFakeService>();
singleton = (FakeService) scope.ServiceProvider.GetService<IFakeSingletonService>();
Assert.False(disposableService.Disposed);
Assert.False(transient1.Disposed);
Assert.False(transient2.Disposed);
Assert.False(singleton.Disposed);
}
Assert.True(disposableService.Disposed);
Assert.True(transient1.Disposed);
Assert.True(transient2.Disposed);
Assert.False(singleton.Disposed);
var disposableProvider = provider as IDisposable;
if (disposableProvider != null)
{
disposableProvider.Dispose();
Assert.True(singleton.Disposed);
Assert.True(transient3.Disposed);
}
}Why would a transient would get disposed in a scoped block at the parent level? If the registration API's intent is to register a transient at the parent level then surely the onus is on the calling code to dispose/release it unless it is encapsulated inside an outer type with a scoped lifestyle?
Here is an example of what I would expect:
var container = new WindsorContainer();
container.Kernel.Resolver.AddSubResolver(new CollectionResolver(container.Kernel));
container.Register(Component.For(typeof(TransientObject)).ImplementedBy(typeof(TransientObject)).LifestyleTransient().Named("Foo"));
container.Register(Component.For(typeof(IOptions)).ImplementedBy(typeof(TransientObject)).LifestyleTransient());
container.Register(Component.For(typeof(IOptions)).ImplementedBy(typeof(ScopedObject)).LifestyleScoped());
container.Register(Component.For(typeof(IOptions)).ImplementedBy(typeof(SingletonObject)).LifestyleSingleton());
container.Register(Component.For(typeof(OptionsContainer)).LifestyleScoped());
OptionsContainer results;
TransientObject outerTransientThatDoesNotGetDisposed;
using (container.BeginScope())
{
outerTransientThatDoesNotGetDisposed = container.Resolve<TransientObject>();
results = container.Resolve<OptionsContainer>();
}
Console.WriteLine($"OuterTransientObject::IsDisposed={outerTransientThatDoesNotGetDisposed.IsDisposed}");
Console.WriteLine($"TransientObject::IsDisposed={results.Get<TransientObject>().IsDisposed}");
Console.WriteLine($"ScopedObject::IsDisposed={results.Get<ScopedObject>().IsDisposed}");
Console.WriteLine($"SingletonObject::IsDisposed={results.Get<SingletonObject>().IsDisposed}");With some output:
OuterTransientObject::IsDisposed=False
TransientObject::IsDisposed=True
ScopedObject::IsDisposed=True
SingletonObject::IsDisposed=False
To summarise, I think your test is wrong. It introduces lifecycle side effects on transients when running in a scoped block without intent or encapsulation. Now before you get angry and close this issue or try to defend your code please consider the implications of tracking scope state across threads, or parent/child scoping which is impossible with this design. What scares me even more, is a whole heap of IoC implementations just went: "Yip, this is ok!". It isn't. Look forward to your response.
Kindest respect.
Gav
Why would a transient would get disposed in a scoped block at the parent level? If the registration API's intent is to register a transient at the parent level then surely the onus is on the calling code to dispose/release it unless it is encapsulated inside an outer type with a scoped lifestyle?
That's not how scopes work and it's not what the test is doing. Scopes are explicit things in this DI container. You can't register things in a scope, but you can resolve things out of it. The scope is basically a container lifetime. I general, we dispose transient disposable objects, when you resolve out of a scope and that scope dies, so does the transient disposable object. Here's the discussion on transient disposables for more context on how it works #456.
In summary resolved instances are per scope are managed per scope/container lifetime.
Now before you get angry and close this issue or try to defend your code please consider the implications of tracking scope state across threads, or parent/child scoping which is impossible with this design.
Can you be more specific? I'm not groking why there are any issues with threading as the scope isn't ambient in this container.
That's not how scopes work and it's not what the test is doing. Scopes are explicit things in this DI container. You can't register things in a scope, but you can resolve things out of it. The scope is basically a container lifetime.
Aaah, that is where my confusion is coming in. Would it be correct then to assume that the creation of a scope creates a child container? I also did not notice an extra call to the IServiceProviderFactory so nothing was telling me that this is effectively a new container. Do you expect that this should be happening?
singleton = (FakeService) scope.ServiceProvider.GetService();
and then:
Assert.False(singleton.Disposed);
Why is it this the case? Any transients inside of this will be disposed, your docs say not to inject scoped dependencies but I am sure people will think it is OK for transients. Should we not simply dispose the lot?
Can you be more specific? I'm not groking why there are any issues with threading as the scope isn't ambient in this container.
I am still not clear on how using nested scopes would work. How would you track state for this? I was looking at the autofac implementation of this and did not notice anything so was a little in the dark as to how I go about this. I have hacked a scope reference onto an AsyncLocal but I dont think this is right and it prevents me from nesting them(so I would have to give it some more thought). I will create child containers though when this happens and see how I go.
Why on earth would you expose a ServiceLocator on your scope? You already have all the registrations telling you what the lifestyles are, you already have the container implementation, why would a scope override this? I really cannot wrap my head around it.
The concept of a ContainerBuilder in IServiceProviderFactory is also weird. Surely it should stand to reason that you don't get to build the container. It gets provided to you. Otherwise you are overriding the container registration API's and again imposing your ServiceCollection registration API's. This is woefully opinionated design. Is the expectation here that the dotnet community rewrites there container registration logic to use your API's?
Why is it this the case? Any transients inside of this will be disposed, your docs say not to inject scoped dependencies but I am sure people will think it is OK for transients. Should we not simply dispose the lot?
Singletons are tied to the root container always. They are also only disposed if the container was responsible for creating them.
I am still not clear on how using nested scopes would work. How would you track state for this?
Scopes aren't nested, they're all top level.
Would it be correct then to assume that the creation of a scope creates a child container?
Basically, it's another universe. You can't add registrations to scopes and it isn't a container builder.
I was looking at the autofac implementation of this and did not notice anything so was a little in the dark as to how I go about this.
Autofact works well because it pretty much mirrors the scope idea in this container. The scope itself is a lifetime and instances are managed there.
Why on earth would you expose a ServiceLocator on your scope? You already have all the registrations telling you what the lifestyles are, you already have the container implementation, why would a scope override this? I really cannot wrap my head around this.
The service provider tracks the lifetime of resolved instances within that scope.
The concept of a ContainerBuilder in IServiceProviderFactory is also weird.
Are you talking about the autofac implementation?
This is woefully opinionated design. Is the expectation here that the dotnet community rewrites there container registration logic to use your API's?
There's actually been lots of discussion about our conforming container in the community (just search for conforming container and ASP.NET Core). There's a proposal for an alternate approach for containers that are incompatible: aspnet/Mvc#5403. Here's the example for Castle Windsor https://github.com/dotnetjunkie/Missing-Core-DI-Extensions/tree/master/src/SampleApplication.CastleWindsor
There's actually been lots of discussion about our conforming container in the community (just search for conforming container and ASP.NET Core). There's a proposal for an alternate approach for containers that are incompatible: aspnet/Mvc#5403. Here's the example for Castle Windsor https://github.com/dotnetjunkie/Missing-Core-DI-Extensions/tree/master/src/SampleApplication.CastleWindsor
@davidfowl - I was hitting a fairly high number of WTF's per minute until you sent me this. It is now less so but still there. I still don't believe scopes should override the original intent of the registration. I also dont believe registration should be fault tolerant. Give me time to absorb what you have sent me, I am closing this issue for now but trust me I will be back. Thanks fella.
Autofact works well because it pretty much mirrors the scope idea in this container.
Autofac pre-dates Microsoft DI by over 8 years, so I'm pretty sure the relationship goes the other way around: Autofac works well because Microsoft DI pretty much mirrors the scope idea from Autofac.
@dotnetjunkie:1, @davidfowl:0 ;)