OnRelease is not being called for singleton instance when container is disposed

[GoogleCodeExporter]

What steps will reproduce the problem?

Following up from my comment at 
http://code.google.com/p/autofac/wiki/OnActivatingActivated

What is the expected output? What do you see instead?

See attached project. I expect to see "One disposed" and "Two disposed" written 
to the console. I only see "One disposed".

What version of Autofac are you using? 

2.6.3.862

Please provide any additional information below.

Original issue reported on code.google.com by sean.fau...@jellyfish.co.nz on 31 Jul 2012 at 9:30

Attachments:

• AutofacTest.zip

[GoogleCodeExporter]

Also, if I add:

container.Resolve<IEnumerable<TestDisposable>>();

before:

container.Dispose();

then "Two" is disposed as expected.

But my understanding is I'm handing ownership over to the container at 
registration time, unless I specify 'ExternallyOwned()'. Surely the instance 
shouldn't have to be resolved to be deterministically disposed when the 
container is disposed?

Original comment by sean.fau...@jellyfish.co.nz on 31 Jul 2012 at 11:22

[GoogleCodeExporter]

Finally, I noticed after adding the resolve above that "One" is then disposed 
twice - once by LifetimeScope.Dispose and then by ComponentRegistry.Dispose. 
However, the instances registered with OnRelease are disposed once only - as 
expected. Just sayin'. ;)

Original comment by sean.fau...@jellyfish.co.nz on 31 Jul 2012 at 11:25

[GoogleCodeExporter]

Original comment by travis.illig on 21 Sep 2012 at 4:38

• Added labels: Module-Core

[GoogleCodeExporter]

*Technically* any IDisposable implementation should be checking for 
double-disposal and only running once anyway... but it's possible to be working 
with incorrectly implemented components, so we can fix it.

Solving half of this - the double-dispose - is pretty easy. The 
ProvidedInstanceActivator wasn't checking to see if it was already disposed 
before running dispose again. Done.

The OnRelease part is a little trickier and may not be something we can (or 
want to) fix.

When you do OnRelease(), internally it is basically saying...
* The component is now ExternallyOwned (no longer "owned" by the lifetime scope 
in which it's registered) - this indicates some other action will be performed 
to dispose the object.
* Once the component has been activated for the first time, we have an 
additional action to run when the lifetime scope is disposing: the action you 
provide.

The catch here is that if the component is never resolved, it's never 
"activated," which means it never really gets 'released' by the lifetime scope, 
either. That means the OnRelease action will never get called.

I'm not sure the OnRelease not-getting-called thing is really a bug. It's doing 
what it was told to do. In a real-world scenario, if someone had a situation 
like this, I'd tell them to implement IDisposable on the type and let things 
"just work" rather than try to fight the semantics around activation and 
release of components here.

Unless folks feel strongly about it... I'll fix the double-dispose and not fix 
the OnRelease-not-getting-called part.

Original comment by travis.illig on 5 Dec 2012 at 11:21

• Changed state: Started

[GoogleCodeExporter]

This may not be quite as straightforward as I thought.

During container disposal, the instance IDisposable.Dispose() method gets 
called directly by the LifetimeScope.Disposer.Dispose() and then the 
ProvidedInstanceActivator.Dispose() method gets called by the 
ComponentRegistry.Dispose() which, in turn, also calls the instance Dispose() 
method.

That means there's really no "single place to check" to see if the instance 
should be disposed (or if it has already been disposed).

The attachment of the provided instance to the LifetimeScope.Disposer appears 
to happen during the InstanceLookup.Activate method, which is basically the 
LifetimeScope.Resolve method.

There's a unit test, though, indicating that we definitely want non-activated 
provided instances to be disposed.

So here's what I'm thinking:
* If the provided instance gets activated, it's going to be cleaned up by the 
owning lifetime scope (whether OnRelease is specified or not).
* If the provided instance doesn't get activated and doesn't have OnRelease 
specified, it should be cleaned up by the activator.
* If the provided instance doesn't get activated and has OnRelease specified, 
no automatic cleanup will happen because no activation == no release.

Original comment by travis.illig on 6 Dec 2012 at 12:30

[GoogleCodeExporter]

This issue was closed by revision 13ac65b50a60.

Original comment by travis.illig on 6 Dec 2012 at 12:44

• Changed state: Fixed

[GoogleCodeExporter]

Implemented changes based on the design noted above. The 
ProvidedInstanceActivator will defer disposal to the owning lifetime scope if 
it gets activated and will do the disposal for non-activated instances if 
instructed to dispose.

Original comment by travis.illig on 6 Dec 2012 at 12:54

[GoogleCodeExporter]

Ownership by itself is an interesting problem. Consider a scenario where the 
container is also being used as a store for owned instances, so a code block 
with access to the container can create and register an instance that will 
outlive it's block scope, where those instances are IDisposable and expected to 
be deterministically disposed when the container is disposed. The instance may 
or may not be needed to be resolved depending on future execution.

It's inconsistent and unintuitive that adding an OnRelease method changes the 
deterministic disposal behavior. To the extent that even if the OnRelease calls 
Dispose (which my sample does) you still can't get back to where you were 
before adding OnRelease because it's now tied to 'activation'. But the 
container will no longer call Dispose for you either!

It seems to me that 'release' should be independent of disposal.

Original comment by sean.fau...@jellyfish.co.nz on 6 Dec 2012 at 1:07

[GoogleCodeExporter]

I'm not sure I understand the example of an IoC container doubling as a store 
for owned instances. If I read it right, it's sort of multi-purposing the 
container to also be a disposal mechanism, where the actual intent of the IoC 
container is simply to invert that control; I might argue that if the point of 
registering an instance is simply to manage its lifetime, there are other ways 
to accomplish that. (For example, you could register those instances directly 
with the lifetime scope's disposer rather than registering them as components.)

I agree there is a level of unintuitive behavior where the notions of "dispose" 
and "release" overlap, but there's also a sort of logic to it.

Specifying "OnRelease" sort of means "My component doesn't actually implement 
IDisposable, or at least it doesn't implement it correctly (as in a WCF proxy) 
so if one of these things gets used, I know better - use my logic instead." The 
key is that whole "if one of these things gets created" notion - if you don't 
resolve/activate a component, it's not really "created." Especially important 
when you look at every other service type - named services, delegate services, 
etc. You tie that special "override the disposal and call my OnRelease" logic 
to the *activated* instance.

From your comment above, "my understanding is I'm handing ownership over to the 
container at registration time, unless I specify 'ExternallyOwned()'" - 
OnRelease() *does* specify ExternallyOwned - the point of which is basically 
saying, "I know best how to manage this thing." It bundles that with an 
automatically executing disposal action that you specify that replaces the 
automatic call to IDispose. But because it's externally owned - you know better 
than us how to dispose it - we don't want to run that method to logically "end" 
the instance unless you resolve it at least once to logically "begin" the usage.

If your component actually does implement IDisposable correctly, you wouldn't 
normally specify OnRelease, which would mean the disposal would just work.

Could this be named better or documented better? Possibly. Is it *technically* 
a solvable problem? Sure. I'm just not sure the changes to the flexibility of 
the internals of Autofac - so things are "special case" around single instances 
that don't get activated but still need to be disposed with custom actions - is 
necessarily justified.

There are a lot of ways to work around it. A couple might be...
* Manually register a disposable wrapper with the Container.Disposer
* Handle the Container.CurrentScopeEnding event

But I don't think there's a good way to solve this specific edge case without 
some fairly significant changes to the whole disposal mechanism that I'm not 
sure we can justify.

We're always open to patches and contributions, though. If you see some easy 
way of taking care of this and can pass it along, by all means please do. It 
may be that I'm missing something or that it wouldn't be as difficult and 
affecting as it appears to me it'd be. Even if you can provide some sort of 
prototype, extension, or something else... We'd be happy to incorporate those 
changes.

Original comment by travis.illig on 6 Dec 2012 at 1:38

[GoogleCodeExporter]

Another workaround that came from a recent commit to the 3.0 code - you could 
register your components as AutoStart() to have them auto-activate.

builder.RegisterInstance(foo).OnRelease(lambda).AutoStart();

I just committed it so it won't be there until the next release, but that 
should work.

Original comment by travis.illig on 14 Dec 2012 at 10:40