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

Dispose services in order #463

Closed
sebastienros opened this Issue Nov 8, 2016 · 4 comments

Comments

Projects
None yet
4 participants
@sebastienros
Member

sebastienros commented Nov 8, 2016

Services should be disposed in the reverse order that they were created.

If service A depends on service B, B will be created first, then A. Now when Dispose() is called on A, it could have to use B. Unfortunately B might already be disposed as the order is not deterministic and based on a dictionary entry. I suggest we enforce the disposal order to be both logical and deterministic as far as the dependency order is defined.

@pakrym

This comment has been minimized.

Show comment
Hide comment
@pakrym

pakrym Nov 11, 2016

Contributor

I have a branch that implements this in our container and adds specification tests but I have couple of concerns:

  1. We are not requiring IServiceProvider to be IDisposable in specification but adding this tests forces it. Neither Autofac not StructureMap implementation of IServiceProvider is IDisposable
  2. Autofac does this in correct order but StructureMap disposes services in the order of creation.
Contributor

pakrym commented Nov 11, 2016

I have a branch that implements this in our container and adds specification tests but I have couple of concerns:

  1. We are not requiring IServiceProvider to be IDisposable in specification but adding this tests forces it. Neither Autofac not StructureMap implementation of IServiceProvider is IDisposable
  2. Autofac does this in correct order but StructureMap disposes services in the order of creation.
@pakrym

This comment has been minimized.

Show comment
Hide comment
@pakrym

pakrym Nov 11, 2016

Contributor

@khellang @nblumhardt what are your thought on this?

Contributor

pakrym commented Nov 11, 2016

@khellang @nblumhardt what are your thought on this?

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Nov 11, 2016

Thanks for the loop-in! This sounds like another case where making assumptions in the framework could cause issues for some container integrations.

Reverse-order disposal is necessary to correctly tear down some component graphs, e.g. where A : IDisposable "registers" itself with B : IDisposable on creation, and "un-registers" on disposal, but:

  • The framework could avoid these kinds of object graphs, or
  • Some additional code could be added, e.g. mediator objects, to make the scenarios work independently of disposal order.

If users want forward-order disposal they should be free to depend on it by selecting a container that implements it - and vice-versa. Personally, I feel reverse-order is the right way to do it, but it's highly unlikely everyone agrees with me ;-)

#433 is a discussion along these lines - disposal order would be a good item to add to the summary on that ticket, I think.

nblumhardt commented Nov 11, 2016

Thanks for the loop-in! This sounds like another case where making assumptions in the framework could cause issues for some container integrations.

Reverse-order disposal is necessary to correctly tear down some component graphs, e.g. where A : IDisposable "registers" itself with B : IDisposable on creation, and "un-registers" on disposal, but:

  • The framework could avoid these kinds of object graphs, or
  • Some additional code could be added, e.g. mediator objects, to make the scenarios work independently of disposal order.

If users want forward-order disposal they should be free to depend on it by selecting a container that implements it - and vice-versa. Personally, I feel reverse-order is the right way to do it, but it's highly unlikely everyone agrees with me ;-)

#433 is a discussion along these lines - disposal order would be a good item to add to the summary on that ticket, I think.

@pakrym

This comment has been minimized.

Show comment
Hide comment
@pakrym

pakrym Mar 17, 2017

Contributor

Fixed by #505

Contributor

pakrym commented Mar 17, 2017

Fixed by #505

@pakrym pakrym closed this Mar 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment