Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypedFactory facility disposes singletons created during factory lifetime #134

Closed
ivan-danilov opened this issue May 20, 2016 · 12 comments
Closed
Labels

Comments

@ivan-danilov
Copy link
Contributor

I guess it was my mistake to make a PR with failing tests without accompanying issue in #61

In short, some instances registered with Singleton lifestyle could be disposed too early and later throw ObjectDisposedException. Like here

@jonorossi jonorossi changed the title ObjectDisposedException from singleton factory TypedFactory facility disposes singletons created during factory lifetime Oct 13, 2017
@jonorossi
Copy link
Member

As @ivan-danilov mentioned in #61:

I believe it is very similar to IOC-345: TypedFactory creates its own release SubPolicy since the breaking change IReleasePolicy interface has a new method: IReleasePolicy CreateSubPolicy(), thus tracks singleton as part of this policy, while it should use parent container's one in order to avoid disposing singletons...

The test right above added one is almost the same except that it uses enclosing SubContainer over singleton factory and my test uses transient factory over singleton factory.

@jonorossi jonorossi added the bug label Oct 13, 2017
@jonorossi
Copy link
Member

This obviously isn't just a problem for singleton components created inside a factory, but all other non-transient lifestyles will get prematurely disposed components.

I think It sounds like the fix would be for transient components created inside the factory to be tracked by the factory's release policy, and for everything else to get the parent IKernel's IReleasePolicy.

I don't know exactly how this would be implemented because TypedFactoryInterceptor.Resolve doesn't have the ComponentModel for the selected component, and it would only support one level deep (i.e. if the (transient) factory resolves a transient which in turn resolves a singleton).

If the factory is a different lifestyle (i.e. scoped) then tracking transients against the factory is a good idea to make sure they always get cleaned up, however what about components of the same lifestyle, should scoped components also get tracked against the factory or leave them for normal lifestyle decommission?

I think we need to decide what the behaviour of this facility should be on components it creates before trying to make a fix.

/cc @ivan-danilov @jberezanski @xlegalles @IanYates @petr-k @ghost @hammett @bindu-purhar @fir3pho3nixx (unsub if you aren't interested anymore)

@ghost
Copy link

ghost commented Oct 14, 2017

I don't know exactly how this would be implemented because TypedFactoryInterceptor.Resolve doesn't have the ComponentModel for the selected component

My experimental PR is a proposal to walk through the flow of information we need in the TypedFactoryInterceptor by exposing a CreationContext using reflection(bad I know!) on the component, this also gives the SingletonLifestyleManager the ability to dump the creation context for disposals(ultimately when the container is disposed). Based on those two signals the TypedFactoryInterceptor can then inspect the existance of the CreationContext and then decide whether to dispose a singleton correctly. Nowhere near a solution yet, but hopefully a template for discussion.

@jonorossi
Copy link
Member

Maybe I'm completely misunderstanding something, but this isn't a problem for the SingletonLifestyleManager, this defect applies to all lifestyles. We need to get the right release policy assigned for those components. What are your thoughts on the rest of my remarks in my last comment about component ownership (therefore decommission)?

@ghost
Copy link

ghost commented Jan 5, 2018

@jonorossi - I will be honest I am not sure I follow. I am not clear on your comments about component "ownership". I feel like I am missing something obvious.

@jonorossi
Copy link
Member

The main point I was making in #134 (comment) is that this problem isn't isolated to singletons, other non-transient lifestyles get caught up with this mess of a different release policy. I was hoping to initiate a discussion on how people are using this facility and how those lifestyles should be handled before trying to work out how to fix it.

Does that help make my comments any clearer?

@ghost
Copy link

ghost commented Jan 22, 2018

@jonorossi - Yes, I think I follow you. I can play around with these release policies, to try and validate this approach.

@ivan-danilov
Copy link
Contributor Author

@jonorossi
I'd say from user's perspective, factories are a way to resolve a component and provide its infrastructure dependencies out-of-band (i.e. code that has a factory is released of the knowledge about infrastructure dependencies of the factory product), leaving only application dependencies to the factory user. In practice, that means that factory might create a bunch of singleton services that should be disposed only when entire container is.

Calling factory.Dispose() on the factory should behave as if every component resolved from it got factory.Release(component) call. It is a convenience call to relieve users from tracking every single object resolved.

A factory should not dictate lifetime of its products - their respective lifetimes should work as usual. After all, when you call container.Release(singletonComponent) - nothing happens, right? When a factory is used to resolve transient dependencies - they should not outlive a factory. In other words, when the factory is scoped or transient - its transient/scoped products should be released along with it. I'm not very versed with scoped components as we don't use them, so I might miss some intricate details between transient and scoped interdependencies. But with singletons and transients, it looks more or less clear to me.

P.S. I use term infrastructure dependencies as it is discussed in this post.

@ivan-danilov
Copy link
Contributor Author

Is it by chance fixed by #439 ?
At least it sounds close enough.

cc @jnm2 @jonorossi

@jnm2
Copy link
Contributor

jnm2 commented Dec 21, 2018

Yes, I do believe it is!

@ivan-danilov
Copy link
Contributor Author

@jnm2 there are unit tests I made in #61 - not sure they add any new checks, but you might consider taking them in even if to double check the issue does not exist anymore.

@jonorossi
Copy link
Member

Thanks, I'll close this issue. I'll leave it to you guys to determine if those unit tests add value on top of the tests added in #439.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants