OnRelease is not being called for singleton instance when container is disposed #383
Comments
|
From sean.fau...@jellyfish.co.nz on July 31, 2012 16:22:05 Also, if I add: container.Resolve<IEnumerable>(); 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? |
|
From sean.fau...@jellyfish.co.nz on July 31, 2012 16:25:54 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'. ;) |
|
From travis.illig on September 21, 2012 09:38:01 Labels: Module-Core |
|
From travis.illig on December 05, 2012 15:21:09 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 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. Status: Started |
|
From travis.illig on December 05, 2012 16:30:23 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:
|
|
From travis.illig on December 05, 2012 16:44:16 This issue was closed by revision 13ac65b50a60 . Status: Fixed |
|
From travis.illig on December 05, 2012 16:54:28 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. |
|
From sean.fau...@jellyfish.co.nz on December 05, 2012 17:07:35 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. |
|
From travis.illig on December 05, 2012 17:38:44 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...
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. |
|
From travis.illig on December 14, 2012 14:40:00 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. |
From sean.fau...@jellyfish.co.nz on August 01, 2012 07:30:57
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.
Attachment: AutofacTest.zip
Original issue: http://code.google.com/p/autofac/issues/detail?id=383
The text was updated successfully, but these errors were encountered: