castleproject / Windsor

Castle Windsor is a best of breed, mature Inversion of Control container available for .NET
http://www.castleproject.org
Apache License 2.0
1.52k stars 456 forks source link

TypedFactory Disposes Before Component That Depends on it #114

Closed pnquest closed 7 years ago

pnquest commented 8 years ago

If a typed factory is a dependency of a disposable component, it is already disposed by the time dispose is called on that component.

If I directly implement the factory myself rather than using the typed factory, everything behaves as I would expect.

This SO Question also details the issue

I have attached a test solution below that shows this behavior (although it is not actually useful itself).

WindsorTypedFactoryDisposalTest.zip

jnm2 commented 8 years ago

Here is another repro, very tiny and to the point: Test Castle Windsor dispose out of order.zip

This is causing our application to crash in production. What's the best workaround?

pnquest commented 8 years ago

@jnm2 I ended up not using the typed factory feature at all and ended up creating my own full factory class instead. When you create your own factory, and wire it up along with everything else it will behave as expected.

jnm2 commented 8 years ago

@pnquest That would be unnecessarily disruptive if there's anything else I can do. I'm using delegate factories and the delegate factories are what is keeping track of releasing the dependencies of the components they provide. I can't provide my own implementation of Func<T> which is also IDisposable. If it's not IDisposable, I don't have a way to track and release all the objects the Func<T> creates.

@hammett Any idea what the ETA might be?

jnm2 commented 8 years ago

I think I found a workaround for the time being- replace dependencyFactory.Invoke() with this.TypedFactoryWorkaround(dependencyFactory):

public static class TypedFactoryWorkaroundExtension
{
    public static T TypedFactoryWorkaround<T>(this IDisposable dependent, Func<T> typedFactory)
    {
        try
        {
            return typedFactory.Invoke();
        }
        catch (ObjectDisposedException)
        {
            // Workaround for https://github.com/castleproject/Windsor/issues/114
            dependent.Dispose();
            throw new OperationCanceledException();
        }
    }
}
pnquest commented 8 years ago

@jnm2 That works fine if you are looking to just gracefully fail, in my case there was still data flowing through that needed to be handled before shutdown could be finalized. In that situation your workaround will not help. I ended up implementing disposing the components the factory created when the factory was disposed as well to handle what sounds like your same problem.

jnm2 commented 8 years ago

@pnquest Exactly.

jnm2 commented 7 years ago

I'm still running into this big time. @hammett Did you make any progress on this?

hammett commented 7 years ago

I'm tracking this at https://github.com/castleproject/Windsor/pull/61 I believe it's the same issue, correct?

jnm2 commented 7 years ago

@hammett I wasn't positive, it's been so long. Is there anything I can do to help crack this one?

hammett commented 7 years ago

Aside from debugging it and fixing it yourself, no. This is new code to me as well since I haven't written that part of Windsor

On Jun 9, 2017, at 1:11 PM, Joseph Musser notifications@github.com wrote:

@hammett I wasn't positive, it's been so long. Is there anything I can do to help crack this one?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jnm2 commented 7 years ago

@hammett I'll be able to try my hand at it in two weeks.

ghost commented 7 years ago

We are thinking of deprecating this facility, please join the discussion at https://github.com/castleproject/Windsor/issues/339

jnm2 commented 7 years ago

@fir3pho3nixx Looks like both of you are thinking of actually keeping this facility which is good, because otherwise I'd have to cut my losses now. 😄

jonorossi commented 7 years ago

Yep, the Typed Factory facility is staying as is UsingFactoryMethod, the Factory Support facility is the one we are looking at deprecating. Reopening.

ghost commented 7 years ago

Whoops wrong factory!

jnm2 commented 7 years ago

@fir3pho3nixx @jonorossi I want to pick your brains on this issue. As a practical matter, things have come to a head with this issue and I've been looking at fixing it.

Simple failing test: https://github.com/castleproject/Windsor/compare/master...jnm2:fix_typed_factory_out_of_order_dispose

Reason: TypedFactoryInterceptor must not be disposed until all handlers which use typed factories are disposed, because the handlers may need to invoke their typed factory dependencies in order to be disposed.

The problem is, DefaultKernel.Dispose contradicts this necessity by doing things in this order:

  1. Calls DisposeComponentsInstancesWithinTracker() which disposes TypedFactoryInterceptor
  2. Calls DisposeHandlers() which disposes a service whose Dispose method invokes a typed factory

I've looked and tried things for a few hours but the complexity of the internals is daunting. What approach would you take to fix this?

https://github.com/castleproject/Windsor/blob/8e139da134c0640dceefb4a8a043af1958e60927/src/Castle.Windsor/MicroKernel/DefaultKernel.cs#L233-L245

jnm2 commented 7 years ago

We need a way to make sure the handler for TypedFactoryIntercepter, created by InterceptorReference.IReference<IInterceptor>.Resolve, ends up in the DisposeHandlers() list instead of tracking the instance in the DisposeComponentsInstancesWithinTracker() list.

That way it could be topologically sorted, so long as the dependencies are reflected properly between it and the handlers that use typed factories.

jnm2 commented 7 years ago

The TypedFactoryInterceptor already does end up in the DisposeHandlers, and it is topologically sorted properly. So we just need a way to keep it from being disposed by DisposeComponentsInstancesWithinTracker, which just releases the container's lifetime policy.

jnm2 commented 7 years ago

Wait... is it really this easy? I thought I had already tried switching the order of the DisposeHandlers and DisposeComponentsInstancesWithinTracker calls, but now it's working!

DisposeComponentsInstancesWithinTracker makes sense as a backstop for anything DisposeHandlers didn't clean up organically via reference counting, and no tests started failing, so I'm satisfied with this...

jnm2 commented 7 years ago

https://github.com/castleproject/Windsor/pull/344

jonorossi commented 7 years ago

@jnm2 great work working through that, now that we've got a unit test that defect is now squished forever.