Burdens are not Release Policy aware from TypedFactories for disposables #373

ghost · 2018-01-12T21:27:20Z

After investigating this issue extensively: #325

Going Dan North on this.

Given:

Type Foo implements IDisposable.
Type Foo is registered via a Typed Factory using a service type with an interface IFoo.
Type Foo is registered outside of a typed factory with the implementing service type Foo.
Type Foo has a life style of transient for both typed facotry and non typed factory registrations.
Type Foo is a late bound component because of the single initial typed factory registration.
Type Foo has IsDefault status via the Typed Factory registration.
The typed factory method for Foo resolves using the windsor microkernel using the delegate component activator.

When:

I resolve IFoo.

Then:

I should have only one instance.
I should only track instances once via the release policy for dispoables.
I should have two burdens from both the typed factory and the non typed factory registration.
I should not have to track sealed non disposables at the parent level that consumes disposable dependencies.
I do care about dependent disposables for top level graph non disposables.

ghost · 2018-01-12T22:02:06Z

@jnm2 - Could you also please review this PR?

ghost · 2018-01-12T22:17:56Z

@aaron-hammond - Could you also please review this PR?

ghost · 2018-01-12T22:25:57Z

src/Castle.Windsor/MicroKernel/Burden.cs

@@ -148,6 +175,7 @@ public void SetRootInstance(object instance)
 			Instance = instance;
 			if (decommission == Decommission.LateBound)
 			{
+				// BOOM!


Smoking gun ...

ghost · 2018-01-12T22:27:35Z

src/Castle.Windsor/MicroKernel/Burden.cs

@@ -89,7 +91,32 @@ public bool RequiresDecommission
 		/// </summary>
 		public bool RequiresPolicyRelease
 		{
-			get { return TrackedExternally == false && RequiresDecommission; }
+			get


We probably need a new abstraction.

ghost · 2018-01-12T22:31:36Z

src/Castle.Windsor.Tests/Registration/UsingFactoryMethodTestCase.cs

@@ -380,7 +380,8 @@ public void Factory_created_sealed_non_disposable_services_with_factory_created_

 			var component = Kernel.Resolve<SealedComponentWithDependency>();

-			Assert.IsTrue(Kernel.ReleasePolicy.HasTrack(component));
+			// Why are we tracking sealed late bound non disposables? This is bonkers. Surely we are only interested in disposable dependencies?
+			// Assert.IsTrue(Kernel.ReleasePolicy.HasTrack(component));


jnm2 · 2018-01-13T00:28:55Z

(Fyi the commit message has two typos, 'facotry' and 'dispoables.')

I don't sense anything wrong about this, though I'm aware that the meat of it may be going over my head a bit. I'd need someone to break down for me the significance of ReleasePolicy_tracking_the_same_ instance_twice_with_transient_lifestyle_and_factory_method_suggests_different_lifestyle.

Why are we tracking sealed late bound non disposables? This is bonkers. Surely we are only interested in disposable dependencies?

I'm not sure what 'sealed' means in this context. Do non-disposable instances need to be tracked so that their potentially disposable dependencies can be released at the appropriate time?

Another question: is it possible to configure an ad-hoc decommission handler on a non-disposable component?

If we track externally, we want to bail out early

Can you help me understand what it means to be tracked externally and how we know for sure that whoever is doing the external tracking has all the necessary information?

Going Dan North on this. Given: Type `Foo` implements IDisposable. Type `Foo` is registered via a Typed Factory using a service type with an interface `IFoo`. Type `Foo` is registered outside of a typed factory with the implementing service type `Foo`. Type `Foo` has a life style of transient for both typed factory and non typed factory registrations. Type `Foo` is a late bound component because of the single initial typed factory registration. Type `Foo` has IsDefault status via the Typed Factory registration. The typed factory method for Foo resolves using the windsor microkernel using the delegate component activator. When: I resolve IFoo. Then: I should have only one instance. I should only track instances once via the release policy for disposables. I should have two burdens from both the typed factory and the non typed factory registration. I should not have to track sealed non disposables at the parent level that consumes disposable dependencies. I do care about dependent disposables for top level graph non disposables.

ghost · 2018-01-13T13:22:04Z

I don't sense anything wrong about this, though I'm aware that the meat of it may be going over my head a bit. I'd need someone to break down for me the significance of ReleasePolicy_tracking_the_same_ instance_twice_with_transient_lifestyle_and_factory_method_suggests_different_lifestyle

This was probably added to help guard against misconfigured lifestyles when helpful exceptions were added. I am not sure it is valid anymore. My reasoning is this:

Factories are late bound components, from what I can tell they need burdens if they are to be tracked. If factories engage the vanilla resolve mechanism from the MicroKernel inside the factory having 2 burdens is unavoidable but you might end up with the same instance for a transient lifestyle. By extending the burdens to be release policy aware and using HasTrack functionality you can allow this to pass for the same resolution context. Maybe this helpful exception was masking over a potential bug?

I'm not sure what 'sealed' means in this context. Do non-disposable instances need to be tracked so that their potentially disposable dependencies can be released at the appropriate time?

You got me there, I will double check for memory leaks with transients. It seems reasonable to think this.

Can you help me understand what it means to be tracked externally and how we know for sure that whoever is doing the external tracking has all the necessary information?

My understanding is when you ask components to be tracked externally you are telling windsor not to wrap them in burdens. The side effect of this if you have to release and dispose yourself.

jnm2 · 2018-01-13T23:29:15Z

Great, thanks for the explanations!

If factories engage the vanilla resolve mechanism from the MicroKernel inside the factory having 2 burdens is unavoidable but you might end up with the same instance for a transient lifestyle.

Sorry to make you explain so much! I'm with you this far, but why is it that engaging the vanilla resolve mechanism results in two burdens? And how is it possible for you to end up with the same instance for a transient lifestyle?

ghost · 2018-01-14T13:11:35Z

Sorry to make you explain so much!

No problem :)

And how is it possible for you to end up with the same instance for a transient lifestyle?

If you have a Foo which takes Bar into it's constructor and Bar implements IDisposable.

Register Bar as transient.
Register Foo as transient
Register IFoo using factory which calls kernel.resolve with IsDefault
Then call container.resolve IFoo

Because the factory calls kernel.resolve inside of it, it is effectively passing back the same transient from registration (2) in (3). Bizarrely you can do this without problems when you remove IDisposable from Bar.

I'm with you this far, but why is it that engaging the vanilla resolve mechanism results in two burdens?

Looks like Windsor is trying to track Burdens for Bar here during the resolution of Foo and finally resolution of IFoo. Because they are the same instance(because of calling resolve inside the factory method), Burdens have trouble tracking this twice. This is what led me to believe that they need to be release policy aware to be able to make use of the HasTrack feature. This was Bar will still get resolved whether or not it implements disposable.

When doing that that though, a pretty important looking helpful exception no longer get's thrown :)

ghost · 2018-01-17T21:41:31Z

Can re-open if we feel necessary.

Ref: https://github.com/fir3pho3nixx/Windsor/pull/8

jnm2 · 2018-01-18T00:31:15Z

Hopefully Krzysztof has some insight. Let me know if you think I might be able to help.

ghost · 2018-01-18T18:39:24Z

@jnm2 - Thanks, I deleted the comment after I posted it. Happy to take this forward with @jonorossi with detailed discussions.

I think you can definitely help! Let me figure this out a bit more first. :)

ghost commented Jan 12, 2018

View reviewed changes

ghost closed this Jan 17, 2018

ghost mentioned this pull request Jan 17, 2018

Facilities SystemWeb, Mvc and WebApi #351

Merged

This was referenced Jan 18, 2018

OWIN-compatible implementation for "PerWebRequest" lifestyle #283

Closed

Order of component decommission upon container disposal when using typed factories #371

Open

ghost mentioned this pull request Oct 1, 2018

Windsor adds a dependency resolved from typed factory as a burden on an unrelated object #436

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Burdens are not Release Policy aware from TypedFactories for disposables #373

Burdens are not Release Policy aware from TypedFactories for disposables #373

ghost commented Jan 12, 2018 •

edited by ghost

Loading

ghost commented Jan 12, 2018

ghost commented Jan 12, 2018

ghost Jan 12, 2018

ghost Jan 12, 2018 •

edited by ghost

Loading

ghost Jan 12, 2018 •

edited by ghost

Loading

jnm2 commented Jan 13, 2018 •

edited

Loading

ghost commented Jan 13, 2018

jnm2 commented Jan 13, 2018

ghost commented Jan 14, 2018 •

edited by ghost

Loading

ghost commented Jan 17, 2018 •

edited by ghost

Loading

jnm2 commented Jan 18, 2018

ghost commented Jan 18, 2018 •

edited by ghost

Loading

Burdens are not Release Policy aware from TypedFactories for disposables #373

Burdens are not Release Policy aware from TypedFactories for disposables #373

Conversation

ghost commented Jan 12, 2018 • edited by ghost Loading

ghost commented Jan 12, 2018

ghost commented Jan 12, 2018

ghost Jan 12, 2018

Choose a reason for hiding this comment

ghost Jan 12, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jan 12, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

jnm2 commented Jan 13, 2018 • edited Loading

ghost commented Jan 13, 2018

jnm2 commented Jan 13, 2018

ghost commented Jan 14, 2018 • edited by ghost Loading

ghost commented Jan 17, 2018 • edited by ghost Loading

jnm2 commented Jan 18, 2018

ghost commented Jan 18, 2018 • edited by ghost Loading

ghost commented Jan 12, 2018 •

edited by ghost

Loading

ghost Jan 12, 2018 •

edited by ghost

Loading

ghost Jan 12, 2018 •

edited by ghost

Loading

jnm2 commented Jan 13, 2018 •

edited

Loading

ghost commented Jan 14, 2018 •

edited by ghost

Loading

ghost commented Jan 17, 2018 •

edited by ghost

Loading

ghost commented Jan 18, 2018 •

edited by ghost

Loading