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

Allow actions to be executed and disposables to be disposed when container gets disposed #664

Closed
dotnetjunkie opened this Issue Feb 14, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@dotnetjunkie
Copy link
Collaborator

dotnetjunkie commented Feb 14, 2019

This is the feature issue for which the problem is described in #519.

Currently, scopes allow delegates to be registered that are executed when the scope is disposed of, e.g.:

scope.WhenScopeEnds(() => { ... });

Scopes also allow the registration of IDisposable implementations that get disposed of when the scope itself is disposed of, e.g.:

IDisposable myScopedImpl = ...;
scope.RegisterForDisposal(myScopedImpl);

The container itself manages a Scope that is used to capture created Singleton instances. Registration of delegates and disposables, however, isn't as easy as it is when using Scope. This use case should be simplified.

There are a few options here:

Option 1: Expose the container Scope

As the container internally maintains a 'singleton scope', this Scope can be exposed. By exposing that Scope, it becomes trivial to register delegates or disposables, as this is the same API (and same object), e.g.:

var container = new Container();
Scope containerScope = container.ContainerScope; // new property of type Scope

container.ContainerScope.RegisterForDisposal(myScopedImpl);
container.ContainerScope.WhenScopeEnds(() => { /* do something useful */ });

container.Dispose();

Advantage of using the same API is that possible extensions, created by the user (such as extension methods) can be applied to both 'request' scopes and the container scope.

Downside of this approach is that this will expose the complete Scope API, including GetItem, SetItem and Dispose. Especially exposing Dispose is a problem, as this will allow this 'container scope' to be disposed, but that would leave the container in a broken state, while it hasn't been disposed. But even GetItem and SetItem can be confusing as the container already contains GetItem and SetItem extension methods. On the other hand, a method such as GetDisposables, will likely be useful at container level, as this makes it possible to asynchronously dispose singletons (which is something that is not easy to do).

Option 2: Expose a new ContainerScope class that mimics the Scope API and forwards the call to the internal container Scope

var container = new Container();
ContainerScope scope = container.ContainerScope; // new property of type ContainerScope

container.ContainerScope.RegisterForDisposal(myScopedImpl);
container.ContainerScope.WhenScopeEnds(() => { /* do something useful */ });

container.Dispose();

This prevents the exposure of methods that are confusing, while allowing that API to evolve separately from the Scope API. It also allows methods to be grouped instead of 'polluting' the main Container API.

Downside of this approach is the introduction of a new type and it might be confusing that the user needs to interact with a container.ContainerScope instead of directly using e.g. container.RegisterForDisposal.

Option 3: Add the required methods directly to the Container API

Instead of introducing a new type and a new property, the methods could also be added to the Container API directly, e.g.:

var container = new Container();

container.RegisterForDisposal(myScopedImpl);
container.WhenContainerDisposes(() => {/* something useful */}); // use different name for clarity
container.GetDisposables(); // this method name might even be confusing

container.Dispose();

The addition of many methods to the Container API, has always been something I tried to prevent. For that reason, the GetItem and SetItem methods are provided as extension methods in the SimpleInjector.Advanced namespace, to prevent cluttering the main API. Having these new methods on the main API does break with this model, so it isn't my preferred approach.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

dotnetjunkie commented Feb 14, 2019

Added feature-664 branch.

@TheBigRic

This comment has been minimized.

Copy link
Collaborator

TheBigRic commented Feb 14, 2019

I like option 3 the most, I think.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator Author

dotnetjunkie commented Mar 13, 2019

I decided to go with the second option, the creation of a new ContainerScope facade that can be used for hooking container-dispose actions. I choose this option because:

  • I really want to prevent the Container API to be 'polluted' with more and more methods, and rather be able to bundle them, as we did with Container.Collection.
  • Compared to option 1, this hides several methods (i.e. GetInstance<T>, GetInstance, and Dispose) that make little sense on this level.
  • Compared to option 3, this option can be completely replaced with option 1 later on, without causing any breaking changes (by letting ContainerScope derive from Scope).

This means that the API will become:

var container = new Container();

container.ContainerScope.RegisterForDisposal(myScopedImpl);
container.ContainerScope.WhenScopeEnds(() => { /* do something useful */ });
container.ContainerScope.GetDisposables();
container.ContainerScope.SetItem("key", new object());
container.ContainerScope.GetItem("key");

container.Dispose();

dotnetjunkie added a commit that referenced this issue Mar 13, 2019

dotnetjunkie added a commit to simpleinjector/Documentation that referenced this issue Mar 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.