Nested Transient TypedFactory Dependency Is Not Disposed When Parent Released

[robsonj]

This seems to be a breaking change between Castle.Windsor v4.1.0 and v4.1.1 possibly related to pull request #439 .

I have attached a unit test which succeeds with Castle.Windsor v4.1.0 (Core 4.2.0) but fails with Castle.Windsor v4.1.1 (Core 4.2.0).

WindsorContainerTypedFactoryTests.cs.txt

[jonorossi]

@jnm2 do you have some time to look at this?

[jnm2]

@jonorossi Sure!

Simplified the repro a bit:

using System;
using Castle.Facilities.TypedFactory;
using Castle.MicroKernel.Registration;
using Castle.Windsor;
using NUnit.Framework;

[TestFixture]
public static class WindsorContainerTypedFactoryTests
{
    [Test]
    public static void NestedTransientTypedFactoryDependencyIsDisposedWhenParentReleased()
    {
        var container = new WindsorContainer()
            .AddFacility<TypedFactoryFacility>()
            .Register(Component.For(typeof(IFactory<>)).AsFactory())
            .Register(Component.For<ParentService>().LifestyleTransient())
            .Register(Component.For<ChildService>().LifestyleTransient());

        var service = container.Resolve<ParentService>();
        container.Release(service);

        Assert.That(
            service.Child.Disposed,
            "transient child typed factory dependency not disposed " +
            "automatically by container when parent released");
    }
}

public sealed class ParentService : IDisposable
{
    public ParentService(IFactory<ChildService> childFactory)
    {
        Child = childFactory.Resolve();
    }

    public ChildService Child { get; }

    public bool Disposed { get; private set; }

    public void Dispose() => Disposed = true;
}

public sealed class ChildService : IDisposable
{
    public bool Disposed { get; private set; }

    public void Dispose() => Disposed = true;
}

public interface IFactory<T>
{
    T Resolve();
    void Release(T service);
}

[robsonj]

So without changes, it passed just fine?

[jnm2]

@robsonj One thing I notice immediately is that your parent service doesn't balance the _childFactory.Create call with a call to _childFactory.Release. I personally would be sure to release in Dispose() everything that had been resolved by the class in its lifetime and not yet released. Because you are resolving out-of-band, I think you need to release out-of-band too.

Imagine if your Child = _childFactory.Create(tag); moved from the constructor to a method you called manually, e.g. Initialize(foo). Then, even Castle.Windsor 4.1.0 would act the same as 4.1.1 is acting, now that it is more consistent.

If the parent service is truly container-agnostic (meaning it isn't coupled to the fact that you're using Windsor as opposed to, say, NInject or poor-man's DI), how would the code that uses the parent service know when to dispose the child service if the parent service doesn't call the factory's release method?
The way it is now, you're relying on the code that uses your parent service to use thread statics to track things differently when you happen to call _childFactory.Create(tag) within the same call stack as the ParentService constructor call. Requiring this kind of stack inspection is not a good API for the parent service to push onto its consumers, even if Castle Windsor happens to be around to shoulder this burden.

(If you want to avoid being responsible to manually release Child in the parent's Dispose method, the balanced thing to do would be for the constructor to stop manually resolving, to take an IService2 dependency instead of an ICreateService2 dependency. As far as my knowledge of Windsor goes, this might not be an option for you because of the factory's tag parameter.)

[jnm2]

@robsonj

So without changes, it passed just fine?

No, the simplified repro I have above behaves identically to your original one. Fails on 4.1.1, passes on 4.1.0.

[jnm2]

There is one other thing that could be done, but it would be a pretty significant change to Castle.Windsor. Instead of a single factory instance being used for all components, a completely new factory instance could be created each time a component is created that depends on it. That way the matching factory can be disposed when the parent component is disposed, releasing all child instances that had ever been resolved through that specific factory instance.

I'm dubious that it would be worth it or that it would encourage good container-agnostic practices. One Release for every Resolve is a good rule of thumb.

[jnm2]

@jonorossi, @stakx Those are my thoughts, what are yours? Does this sound right to you?

[jonorossi]

@jnm2 thanks for looking into this, I hadn't even opened the code until now.

I agree, in this example the child service (in 4.1.0) was incorrectly captured into the parent service's burden just because it happened to resolve inside the stack of the resolution context of the parent service. With your fix it correctly makes factories responsible for their own lifetimes and makes things consistent whether you resolve inside the child service's constructor like this example or at some later time. Windsor has always been designed that if you explicitly call Resolve you must also explicitly call Release.

I assume you get the intended decommission behaviour if you dispose the singleton factory or the whole container?

[jnm2]

@jonorossi No problem. Thanks for confirming!

Disposing the whole container does dispose the child service, but I'm not sure how to directly dispose the singleton factory since in order to get a reference to it I have to resolve it, and then the only thing I can do is release it.

[jonorossi]

I'm not sure how to directly dispose the singleton factory since in order to get a reference to it I have to resolve it, and then the only thing I can do is release it.

Of course silly me, it is a singleton, it lives until the end of the container. A different lifestyle could be tested, but that isn't the problem raised here.

[jonorossi]

@robsonj are you happy with the explanation here?

@jnm @robsonj do we need to make anything clearer in the docs?

[robsonj]

Thanks for looking at this. In code, we don't explicitly resolve the factory, that was mainly for convenience in the unit test. I've updated the unit test al a @jnm2 example, which still passes now on 4.1.0 when not explicitly resolving the factory, I'll give updating to 4.1.1 another shot.

Again, thanks for the help and fast response, very much appreciated!

[jnm2]

@robsonj (just to clarify) we're not saying that you should copy the simplifications I did in my simplified repro, but rather that your parent service (Service1) should be balancing each call to _childFactory.Create with a call to _childFactory.Release, which would probably mean adding a _childFactory.Release(Child); statement inside Service1.Dispose.

[robsonj]

Yes, but I think your refactored unittest better reflects what we're doing in the real world than the original unit test

[robsonj]

Sorry, I'm an idiot, your refactored unit test still fails under 4.1.1 versus 4.1.0, You are correct, the parent should be disposing/releasing the child in its dispose... which is what we do in real life. I've adjusted my test

[jnm2]

Don't worry, I definitely have those days too. Ideally Windsor wouldn't have been erroneously releasing back when the code was first written, causing the confusion. =)