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

Child container doesn't dispose all its resolved components on disposal #324

Closed
reijerh opened this issue Sep 11, 2017 · 8 comments
Closed

Comments

@reijerh
Copy link

reijerh commented Sep 11, 2017

More specifically: child containers don't seem to dispose components that were resolved through them, but were registered in the parent container.

Original issue: #27
It seems the core issue this ticket was about wasn't really fixed, at least the bug is still there in 4.0.

Here's a unit test (adopted from @mickdelaney's fork mickdelaney@7e8ab35):

[Test]
public void DisposeChildContainerComponentsResolvedAgainstParentContainer()
{
	var parentContainer = new WindsorContainer();

	var childContainer = new WindsorContainer();
	parentContainer.AddChildContainer(childContainer);
	parentContainer.Register(Component.For<DisposableFoo>().LifestyleTransient());

	var first = childContainer.Resolve<DisposableFoo>();
	Assert.IsNotNull(first);
	childContainer.Release(first);
	Assert.AreEqual(1, DisposableFoo.DisposedCount);

	var second = childContainer.Resolve<DisposableFoo>();
	Assert.IsNotNull(second);
	childContainer.Dispose();
	Assert.AreEqual(2, DisposableFoo.DisposedCount);
}

Output:

Expected: 2
But was: 1

(NB: the original unit test registered the component as Singleton, meaning the test would already fail at the first case)

It seems that if we register the DisposableFoo at the childContainer instead, the object is disposed correctly.

You can see in the debugger that the object is tracked by the parentContainer rather than by the childContainer.

@ghost ghost mentioned this issue Sep 13, 2017
@ghost
Copy link

ghost commented Sep 13, 2017

@reijerh - This one was a tough nut to crack but I think I might have fixed it.

Your test is here and it now passes.

@reijerh
Copy link
Author

reijerh commented Sep 13, 2017

Thanks for the very quick response! I couldn't figure it out myself as an outsider due to the complex nature of the project. Will try out the fix when a new version is released.

@ghost
Copy link

ghost commented Sep 13, 2017

@reijerh - No problem.

Once @jonorossi reviews the PR and we clean it up a little we will get this deployed for you.

@jonorossi
Copy link
Member

The problem here is that that component is only indirectly resolved via the child container, the component is registered, resolved, tracked and should be released in the parent container.

I've added a review comment to #335, that fix will break many scenarios, unfortunately the fix for this won't be so easy, as you can see child containers haven't really been designed, they aren't really recommended in their current state because of many limitations.

Windsor however has followed the rule that if you call Resolve you must also call Release even for transients, which would fix this unit test, but obviously not your expectations of Dispose.

@reijerh could you share how you are using child containers because an IHandlerSelector (allowing alternating what gets resolved) or a scoped lifestyle are nearly always a better solution.

@ghost
Copy link

ghost commented Sep 20, 2017

Please join us on #338 for more discussion

@reijerh
Copy link
Author

reijerh commented Sep 20, 2017

I commented regarding the use case in #338, should probably continue discussion there.

@ghost
Copy link

ghost commented Sep 21, 2017

@reijerh - I find it very interesting the way the discussion is going.

I had a go at fixing your bug and I must agree with @jonorossi after he reviewed it. It is going to fix your problem and ruin the party for everyone else. This is why I want the damn thing removed! :)

I have posted a way of achieving this without even using the parent/child container API Windsor.
Please see: https://github.com/fir3pho3nixx/Windsor.Plugins

I really do not believe this API is needed and it conforms to your requirement of not polluting the parent container. In fact parents, do not need to know about child containers.

Would love to hear some feedback from you on this approach. It does use releasetracking/burdens(something that was asked to be removed, not sure I agree just yet). The only difference is I did adhere to the resolve/release rule. You are not doing that. Doc update? Assuming you clearly inherited this app.

Maybe we can materialise something useful in the way of a facility if you are open to collaborating on this.

@ghost
Copy link

ghost commented Sep 21, 2017

PS: My approach also fixes this bug :)

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

No branches or pull requests

2 participants