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

ObjectDisposedException in child lifetime scopes of lifetimeScope activated with configurationAction #1020

Closed
BigApple1988 opened this issue Aug 27, 2019 · 7 comments · Fixed by #1061

Comments

@BigApple1988
Copy link

I've reproduced a situation related to #971
Actually it's related indirectly to async code and unobserved tasks.
You can reproduce the error by writing simple test

        [Fact]
        public void NestedLifetimeScopesShouldNotBeDisposed()
        {
            var builder = new ContainerBuilder();
            builder.RegisterType<object>();
            var rootContainer = builder.Build();
            var parentLifetimeScope = rootContainer.BeginLifetimeScope(x => x.RegisterType<object>());
            var childLifetimeScope = parentLifetimeScope.BeginLifetimeScope();
            parentLifetimeScope.Dispose();
            var exception = Record.Exception(() => childLifetimeScope.Resolve<object>());
            Assert.Null(exception);
        }

I ran into this exception in my ASP.NET Web Api service with Owin middleware. There is a code, that begins new LifetimeScope and adds IOwinContext for each request by passing configurationAction to BeginLifetimeScope method
https://github.com/autofac/Autofac.Owin/blob/af5911b7163b301aa347adb45d5a703f8caa4ce0/src/Autofac.Integration.Owin/AutofacAppBuilderExtensions.cs#L390
So if you start lifetimeScope eg in your contoller and run a new Task in it, you have ObjectDisposedException in all nested lifetime scopes right after your "synchronous" code ends.

Code that throws exception was added in 0f01618
There is no pull request and I don't know if this behaviour is by design, but it worked in the past and there is no descriptions of this behaviour in any documentation (or I didn't find it).

I'll try to look and the code and propose a pull request if you say it's a bug

@tillig
Copy link
Member

tillig commented Aug 27, 2019

Based on your example, you're disposing the parent out from under a child. You can't do that. Object sharing (e.g., "instance per request," "single instance," etc.) go up to the root container based on hierarchy. If you dispose something in the middle of the hierarchy, you break that. That's not a bug, that's a need for better lifetime scope management in the application.

@BigApple1988
Copy link
Author

I'll give another example.

       private class RootDependency
        {
        }

        private class NestedDependency
        {
        }

        [Fact]
        public void This_Code_Does_Not_Throw()
        {
            var builder = new ContainerBuilder();
            builder.RegisterType<RootDependency>().InstancePerLifetimeScope();
            var rootContainer = builder.Build();
            var parentLifetimeScope = rootContainer.BeginLifetimeScope(x => x.RegisterType<NestedDependency>());
            var childLifetimeScope = parentLifetimeScope.BeginLifetimeScope();
            parentLifetimeScope.Dispose();
            var exception = Record.Exception(() => childLifetimeScope.Resolve<RootDependency>());
            Assert.Null(exception);
        }

        [Fact]
        public void This_Code_Does_Throw()
        {
            var builder = new ContainerBuilder();
            builder.RegisterType<RootDependency>().InstancePerLifetimeScope();
            var rootContainer = builder.Build();
            var parentLifetimeScope = rootContainer.BeginLifetimeScope(x => x.RegisterType<NestedDependency>());
            var childLifetimeScope = parentLifetimeScope.BeginLifetimeScope();
            parentLifetimeScope.Resolve<RootDependency>();
            parentLifetimeScope.Dispose();
            var exception = Record.Exception(() => childLifetimeScope.Resolve<RootDependency>());
            Assert.Null(exception);
        }

        [Fact]
        public void But_This_Code_Does_Not_Throw_Instead_Of_Resolve_RootDependency_Before_Dispose_Parent_LifetimeScope()
        {
            var builder = new ContainerBuilder();
            builder.RegisterType<RootDependency>().InstancePerLifetimeScope();
            var rootContainer = builder.Build();
            var parentLifetimeScope = rootContainer.BeginLifetimeScope();
            var childLifetimeScope = parentLifetimeScope.BeginLifetimeScope();
            parentLifetimeScope.Resolve<RootDependency>();
            parentLifetimeScope.Dispose();
            var exception = Record.Exception(() => childLifetimeScope.Resolve<RootDependency>());
            Assert.Null(exception);
        }

In my opinion test 2 and 3 are equal (because NestedDependency does not affect RootDependency registration)
so as test 1 and 2 are equal. But test 2 throws exception because RootDependency was resolved in parent lifetimeScope before disposal, test 1 doesn't throw because RootDependency was never resolved. test 3 doesn't throw because there is used BeginLifetimeScope overload without configurationAction parameter (I don't know why it affects).
I would be happy to use scenario No 3, but Autofac.Owin library force me to live with scenario No 2.
Shoud I create an issue in Autofac.Owin repo to change behaviour of IOwinContext registration?

@tillig
Copy link
Member

tillig commented Aug 27, 2019

I'm not sure I totally follow, perhaps one of the other team members here can allocate more time to this than I can.

However, I see in all three cases there is a parent lifetime scope being disposed out from under a child lifetime scope and that's not a supported scenario - no guarantees on whether that will or won't work. If something needs to change to ensure a parent isn't disposed out from under you (e.g., if the lifetime scope for the request is being disposed before the actual request is finished), that's something we can fix.

If you're doing something like launching some sort of async action on your own that runs longer than the request or launches outside the request scope, that's not supported and you're going to have to figure out how to fix that in your app code. It is intentional that the request lifetime scope is tied to a request; if you start a task that isn't part of the request and isn't synchronized in a way that ends before the request ends, you're already in pretty unsupported territory.

Again, though, I don't have time to allocate at the moment to setting up a repro or digging deep into the whys and wherefores on this one. I'll just say... if you're doing something like I mentioned above, you're kinda on your own. And if you're not, maybe providing a more complete repro that actually shows a task being launched in the unit test and illustrating exactly what's going on may help.

@BigApple1988
Copy link
Author

documentation says Child Scopes are NOT Automatically Disposed (https://autofac.readthedocs.io/en/latest/lifetime/disposal.html#child-scopes-are-not-automatically-disposed)
I thought so as child scope registrations... I see no reason not to dispose child scopes if you can do nothing with them after parent scope disposal.

Thanks for your help and answers! I'll try to investigate an issue more carefully and provide more complete repro with async code

@tillig
Copy link
Member

tillig commented Aug 27, 2019

Child scopes aren't automatically disposed, that's 100% true. It's because we don't track references from parent to child. We DO track child to parent, so we can walk back to the root container, but we ran into trouble trying to keep references (even weak references) from parent to child and trying to dispose everything in a hierarchy for you. Thus, the responsibility to dispose a scope is on the creator of that scope. That includes making sure you stop using and dispose of a child scope before its parent is disposed. Given the root container is, itself, a lifetime scope, that'd be like saying, "If you create a request lifetime scope and then dispose the container, the request lifetime scope doesn't work right anymore." Of course it doesn't.

@BigApple1988
Copy link
Author

I can't argue with that because it sounds very reasonable. but I think i'd like to have one expected behaviour (even though it would be unconditional exception) in all three cases I provided below, when you trying to resolve some dependency from child lifetime scope if it's parent was already disposed. now it 's a bit confusing when in some cases you can resolve dependency and in some cases (maybe not under your control, like in the Owin context registration case) you aren't

@tillig
Copy link
Member

tillig commented Aug 27, 2019

Fair enough. Open to suggestions or PRs on how to handle that. As long as we're not maintaining a reference where the parent is tracking child scopes (way too easy to tank perf or memory leak things inadvertently when we tried that) and we don't add perf overhead (e.g., no additional locking to check if a parent scope is disposed prior to resolving things in a child scope) then I'm fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants