Fixed components resolved from typed factories being disposed along with unrelated objects #439
Conversation
| @@ -51,7 +51,7 @@ public interface IKernelInternal : IKernel | |||
|
|
|||
| IDisposable OptimizeDependencyResolution(); | |||
|
|
|||
| object Resolve(Type service, IDictionary arguments, IReleasePolicy policy); | |||
| object Resolve(Type service, IDictionary arguments, IReleasePolicy policy, bool ignoreParentContext = false); | |||
There was a problem hiding this comment.
Adding this parameter is a breaking change to implementors of IKernelInternal. Given the name IKernelInternal, I'm assuming users are expected to not be implementing this interface.
There was a problem hiding this comment.
I'm assuming users are expected to not be implementing this interface.
Correct, it is there because Castle used to have two containers, the MicroKernel and Windsor, the two got merged but obviously still visible from the code base.
| return ResolveComponent(handler, service, additionalArguments, policy, ignoreParentContext: false); | ||
| } | ||
|
|
||
| private object ResolveComponent(IHandler handler, Type service, IDictionary additionalArguments, IReleasePolicy policy, bool ignoreParentContext) |
There was a problem hiding this comment.
The original protected method above can be deleted if you don't think users will have inherited from DefaultKernel and called ResolveComponent(IHandler, Type, IDictionary, IReleasePolicy) from inside.
|
Sorry for the delay, I'll check this out tomorrow. |
jnm2
changed the title
jnm2
force-pushed
the
typed_factory_burden_on_unrelated_object
branch
from
a5b34ef
to
73a7a7f
Compare
|
Started a 'fixes' section in the changelog and removed whitespace trimming per @stakx's feedback on my other PR. |
Yes, this appears a like a sound change, I'm not familiar enough with the typed factory code to know if this could be done a little cleaner, but I'm happy for the change to go in. At least we've got a unit test for it now, and it knocks one typed factory defect off.
I've created a 4.1 branch off v4.1.0, switch this pull request over to that base and we can get the change out. I'll get the change merged back into master. |
I'm using |
I know the editorconfig extension in VS Code would start behaving. I'd have to check on ReSharper/VS. |
|
Thanks so much! 🎉 Switched branches. Re: how clean of a change it is. I don't know either. My rationale is that the per-call-stack tracking is not appropriate (it's the cause of the problem) and that this is the simplest way to turn it off when it's not appropriate. |
You'll need to rebase too, otherwise this pull request will drag 61 commits from master across. |
jnm2
force-pushed
the
typed_factory_burden_on_unrelated_object
branch
from
73a7a7f
to
df7b635
Compare
|
@jonorossi Done, sorry! Did I update the changelog properly? |
|
Changelog looks good. Thanks. |
|
I'm trying to upgrade from Castle.Windsor 4.1.0 to 4.1.1 and seeing a unit test failure between the two versions, the second assert fails in the following unit test for 4.1.1, is this expected behavior? |
|
@robsonj could you please create a new issue referencing this pull request. |
|
Opened Issue #451, thank you. |
Fixes #436.
@jonorossi I'm pretty confident that this is the right change to make. The fact that the factory is resolving in the call stack of the ShortLivedViewModel constructor call is accidental. Windsor should be acting the same as if the factory was resolving the component after a short delay (no longer in a component's constructor call stack). I added a comment to document this. Do you agree with this assessment?
A 4.1.1 release would be nice but in the meantime we'll gladly use the AppVeyor feed!