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

ExportFactory throws container disposed exception #380

Closed
yallie opened this issue Feb 26, 2021 · 12 comments
Closed

ExportFactory throws container disposed exception #380

yallie opened this issue Feb 26, 2021 · 12 comments
Assignees

Comments

@yallie
Copy link
Contributor

yallie commented Feb 26, 2021

This issue continues #378 topic. Here is the code:

public interface ISessionManager { void CreateSession(); }

[Export(typeof(ISessionManager)), PartCreationPolicy(CreationPolicy.Shared)]
public class SessionManager : ISessionManager
{
    [Import]
    private ExportFactory<IHelper> HelperFactory { get; set; }

    public void CreateSession()
    {
        // the next line works only for the first time, but the next time it fails
        using var export = HelperFactory.CreateExport();
    }
}

It works with MEF when using container hierarchy (one root application container, and each request creates a child container for non-singleton exports), and it works with DryIoc v4.7.3, but fails with DryIoc v4.7.4.

yallie added a commit to yallie/DryIoc that referenced this issue Feb 26, 2021
dadhi pushed a commit that referenced this issue Feb 27, 2021
* Demonstrates ExportFactory failure on the second and subsequent resolution requests, issue #380.

* Avoid using latest C# using syntax to be compatible with Appveyor CI server.
@dadhi
Copy link
Owner

dadhi commented Mar 1, 2021

@yallie Hello,

To better understand the issue (as well as #378 ) I wanted to get back to the reason behind using WithDefaultReuse(Reuse.Scoped).

In this setup in #378 you will get the singleton DecoratedService with the dependency on Lazy<IDependencyService> which is Scoped due to this rule. From my point of view, it does not make sense.
But you are applying the rule WithoutThrowIfDependencyHasShorterReuseLifespan to prevent the error,
and this prevents the exception but still... IDependencyService will be resolved once.

@yallie
Copy link
Contributor Author

yallie commented Mar 1, 2021

Hi Maksim!

To better understand the issue (as well as #378)
I wanted to get back to the reason behind using WithDefaultReuse(Reuse.Scoped).

I believe it all came from the old MEF setup.

The app had a container hierarchy: root application container and child request-level containers.
MEF containers automatically track disposable objects and release them on container disposal.
When ported to DryIoc, the app used request scopes to track disposable objects just like MEF.

But you are applying the rule WithoutThrowIfDependencyHasShorterReuseLifespan to prevent
the error, and this prevents the exception but still... IDependencyService will be resolved once.

I think that singleton service uses its scoped dependency once, during the initialization.
When the singleton is first resolved, it initializes, uses its dependency, and then runs on itself.
The dependency is also used by other non-singleton services where it doesn't cause any problems.
It doesn't look very good, I know...

Other singletons use ExportFactory to create tracked scoped dependencies on demand.
Consider a method that's always called from a request scope. It creates a dependency as needed
and adds it to the current scope. When the request finishes, all dependencies are disposed of.
Something like this:

[Export(typeof(ISessionManager)), PartCreationPolicy(CreationPolicy.Shared)]
class SessionManager : ISessionManager
{
    [Import]
    private ExportFactory<IDbHelper> DbHelperFactory { get; set; }

    // this method is always called from a request scope
    public void ValidateSession(ISession session)
    {
        using (var export = DbHelperFactory.CreateExport())
        {
            // The db helper is put in the current request scope
            // it creates the connection, starts the transaction, etc.
            // when disposed, it commits the transaction and closes the connection
            var db = export.Value;
            db.ExecuteSql("select count(*) from sessions where ...");
        }
    }
}

This setup worked with DryIoc v2, v3 and v4.7.3.

@dadhi
Copy link
Owner

dadhi commented Mar 2, 2021

@yallie

When the singleton is first resolved, it initializes, uses its dependency, and then runs on itself.

Does it mean that the dependency should be created in the current opened request scope when used first from the singleton.. or the scope itself does not matter, and the new scope maybe opened just to enable the dependency creation (just like the ExportFactory does)?
The latter is also similar to the Autofac Owned dependency https://autofaccn.readthedocs.io/en/latest/advanced/owned-instances.html

@yallie
Copy link
Contributor Author

yallie commented Mar 2, 2021

Does it mean that the dependency should be created in the
current opened request scope when used first from the singleton.

I thought it was the behavior of previous DryIoc versions.

or the scope itself does not matter, and the new scope maybe opened
just to enable the dependency creation (just like the ExportFactory does)?

Yes, probably :)
Is it easier to implement?

@dadhi
Copy link
Owner

dadhi commented Mar 2, 2021

I thought it was the behavior of previous DryIoc versions.

I don't remember in detail in respect of this case, but maybe and there were reasons to change that.

Yes, probably :)
Is it easier to implement?

Yes, it is as simple as Setup.With(opensResolutionScope: true)

To explain this, let's get back and see how is decorated singleton is injected under the scope (in pseudocode):

scope => new Decorator(
    scope.GetRootOrSelf()
        .Resolve<IDecorated>())

We are getting the root (top parent container) from the scope first, then we are using it to Resolve a singleton, because its lifespan is match for the lifespan of te container, and not for the shorter scope. In turn, it means the IDecorated dependencies will be injected in the context of the root container - make sense because their lifespans should match to the dependency holder or should be disregarded completely (as for Transient).
But that's mean we are outside of the any open scopes in the singleton territory.

On the other side opensResolutionScope will introduce the new scope on the spot like this:

root => new Decorated(
    root.OpenScope(disposeWithParent: true)
        .Resolve<AScopedDependency>())

So AScopedDependency may inject the other scoped services in the same opened-on-the-spot scope. It is very similar to how ExportFactory is working now.

Btw, the scopes behave differently if you switch to the ambient scope context.

@yallie
Copy link
Contributor Author

yallie commented Mar 2, 2021

Hi Maksim, thanks for the explanation!

In turn, it means the IDecorated dependencies will be injected in the context of the root container

Ok, so the lifetime of the singleton dependencies matches the singleton itself. Totally makes sense.
But breaks the backwards compatibility in my case, I'm afraid. Will need to investigate it further.
Perhaps I'd document the current behavior as the correct one rather than try to emulate DryIoc v2/3.

Anyway, for singletons that access their dependencies more than once, the app uses ExportFactory.
It should be fine because ExportFactory<T> like Owned<T> controls the dependency lifetime explicitly.

Btw, the scopes behave differently if you switch to the ambient scope context.

You mean AsyncExecutionFlowScopeContext, right?
I don't see it's used anywhere except for Mvc/Owin/WebApi integrations, so I was totally unaware of it.
What would be the difference for singleton dependencies if ambient scope were used?

@dadhi
Copy link
Owner

dadhi commented Mar 2, 2021

You mean AsyncExecutionFlowScopeContext, right?
I don't see it's used anywhere except for Mvc/Owin/WebApi integrations, so I was totally unaware of it.
What would be the difference for singleton dependencies if ambient scope were used?

Yes, I mean this, and I would rather avoid using conext because it is the shared state context where the Current opened scope may be replaced by another one by third party. It means that in r.CurrentScope.Resolve<Bla>() the actual current scope may change between invocations.
The modern Asp .Net Core moved away from it for good, the same way as it is moved away from the shared HttpContext.Current.
I would suggest you to read the doc on it and maybe experiment. Your cases are complex and it is hard to see all corners here.

For now, I think it is better to revert my change in v4.7.4 until I fully can grasp the consequences and possible alternatives.

@yallie
Copy link
Contributor Author

yallie commented Mar 2, 2021

Yes, I mean this, and I would rather avoid using conext because it is the shared state
context where the Current opened scope may be replaced by another one by third party.
It means that in r.CurrentScope.Resolve() the actual current scope may change between invocations.

Oh, ok. Wouldn't try use it then :)

For now, I think it is better to revert my change in v4.7.4 until I fully can grasp the
consequences and possible alternatives.

The change in v4.7.4 addressed the issue of inconsistent resolution where undecorated service
would resolve and decorated would throw. Are you going to completely revert all the changes
so such resolution is inconsistent again? Apart from ExportFactory issue all looks quite good now...

@dadhi
Copy link
Owner

dadhi commented Mar 2, 2021

The change in v4.7.4 addressed the issue of inconsistent resolution where undecorated service.

Now I am not sure. Would love to not revert - it is always confusing.
Ok, I will try to play with it more, may be finding some middle ground or conditions where both cases are working.
Let's keep this issue open until I find more.

dadhi added a commit that referenced this issue Mar 4, 2021
@dadhi dadhi self-assigned this Apr 28, 2021
yallie pushed a commit to yallie/DryIoc that referenced this issue Apr 30, 2021
@dadhi
Copy link
Owner

dadhi commented May 10, 2022

@yallie Is it still an issue or it could be closed?

@yallie
Copy link
Contributor Author

yallie commented May 11, 2022

Hello Maksim,

I believe it's OK now! :)
The unit test is currently enabled GHIssue380_ExportFactory_throws_Container_disposed_exception.cs.
So if it passes, then the issue is fixed 🎉


By the way, looks like Appveyor doesn't show all test results.
As far as I remember, DryIoc has a couple thousand unit tests, but Appveyor shows only 168 test results.
And test results don't have GHIssue380 test listed:

image

@dadhi
Copy link
Owner

dadhi commented May 11, 2022

@yallie Hi. Thanks for confirming. For some reason AV displays only the XUnit tests and not the NUnit tests, but the latter should be visible in the console output.

@dadhi dadhi closed this as completed May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants