z4kn4fein / stashbox

A lightweight, fast, and portable dependency injection framework for .NET-based solutions.
https://z4kn4fein.github.io/stashbox
MIT License
141 stars 10 forks source link

Child containers improvements #135

Closed adambajguz closed 1 year ago

adambajguz commented 1 year ago

While working with child containers I found three problems:

1. Child containers are not being automatically disposed with parent container.

In my opinion, when a parent contianer is disposed the usage of child contianers is pointless because disposed services may be resolved again. I'd suggest to store a list of child containers in StashboxContainer and dispose child containers when parent is disposed.

Optionally a configuration property bool DisposeChildContianersWithParentContainer can be added to ContainerConfiguration.

2. It's not possible to resolve services ignoring the parent container.

When using the StashboxContainer as a DI container for multitenant app that utilizes generic host, it's not possible to easily activate IHostedService from child containers during host startup.

The biggest problem is to prevent resolving of IHostedService from root/partent container - each IHostedService must be called once. - EDIT 31.05.2022: this is not true, see my comment below

3. It's not possible to resolve services from parent and all its child containers.

Combining problem 1. and 2. it will be much easier in some scenarions to resolve all IHostedService services simply by calling a resolve method from root container and get all services registered in root container and its children (or just in children/predecessors).

[Flags]
public enum ResolutionBehavior 
{
    /// <summary>
    /// Indicates that services should be resolved from current and parent container.
    /// </summary>
    Default = Parent | Current,

    /// <summary>
    /// Indicates that services should be resolved from parent container of the current contianer.
    /// </summary>
    Parent = 1 << 0, // Alternatively Ancestors instead of Parent

    /// <summary>
    /// Indicates that services should be resolved from current.
    /// </summary>
    Current = 1 << 1,

    /// <summary>
    /// Indicates that services should only be resolved from child containers of the current container.
    /// </summary>
    Children = 1 << 2 // Alternatively Predecessors instead of Children
}
using (var rootContainer = new StashboxContainer())
{
    // register rootContainer services here

    var childContainer = rootContainer.CreateChildContainer();
    // register childContainer services here

    IEnumerable<IHostedService> allHostedServices = 
        rootContainer.Resolve<IEnumerable<IHostedService>>(ResolutionBehavior.Current | ResolutionBehavior.Children);

    IEnumerable<IHostedService> predecessorsHostedServices = 
        rootContainer.Resolve<IEnumerable<IHostedService>>(ResolutionBehavior.Children);

} // childContainer is disposed automatically with rootContainer
z4kn4fein commented 1 year ago

Hi @adambajguz, thank you for the suggestions!

  1. This is a reasonable request. It could be easily achieved by accepting an attachToParent bool parameter in the CreateChildContainer() method similar to BeginScope(). This way, each child creation could signal that the created container should be disposed on the parent's disposal. If the parameter is true, the created container could be added to the tracked for dispose objects of the container's root scope.
  2. This one is also reasonable, I see the case where it would be nice to ignore the parent container during a resolution request coming from a child.
  3. This part is raising some questions. Jumping down to a child from a parent's request is not clear to me. If something exists in several child containers, which one should be picked starting from a parent?
adambajguz commented 1 year ago

Answer to 3 I was thinking more about resolving to IEnumerable<> and forgot about a scenario when a single service needs to be resolved. In single service scenario, probably the behavior should be configurable or simply forbidden.

IEnumerable<> problem Also today I've discovered that child containers do not work as I expected. When resolving with IEnumerable<> only services from child container are returned (parent is ommited). This actually is documented: When a dependency is missing from the child container during a resolution request, the parent will be asked to resolve the missing service. If it's found there, the parent will return only the service's registration, and the resolution request will continue with the child. Also, child registrations with the same [service type](https://z4kn4fein.github.io/stashbox/docs/getting-started/glossary#service-type--implementation-type) will override the parent's services. but is not natural for me.

I don't know why I previously thought that when IEnumerable<> is used, services are resolved from both child and parent. This is a huge info for me that is actually problematic. Imagine having some Microsoft.Extensions.Options.IConfigureOptions<MyOptions> registered in both parent and child. I'd expect that when IConfigureOptions<MyOptions> is resolved, the instances will come from both child and parent.

z4kn4fein commented 1 year ago

Thank you for the clarification! I see your point that the services in the parent should be included in IEnumerable<> requests coming from a child.

Let me think through this. I'd like to start working on a solution including your previous points (1. & 2.). I see that you started working on a related PR, would you like to continue the work on that or rather wait for my proposal?

adambajguz commented 1 year ago

@z4kn4fein I started working but after today's discovery the "IEnumerable<> problem" from previous comment, I feel that I need your help. You can reuse my proposal or start from scratch :)

adambajguz commented 1 year ago

I see your point that the services in the parent should be included in IEnumerable<> requests coming from a child. I think this should be configurable in case someone needs the old behavior.

z4kn4fein commented 1 year ago

I think this should be configurable in case someone needs the old behavior.

Yep, I think your approach with the ResolutionBehavior would also fit there to control this.

z4kn4fein commented 1 year ago

@adambajguz, I've published v5.10.0-preview-808 with my proposal, which contains the following:

  1. The CreateChildContainer() method got an attachToParent parameter that signals whether the child should be disposed along with the parent. It's true by default.
  2. Each Resolve() method now accepts a ResolutionBehavior (the same one you introduced in your PR) that could indicate which container level can take part in the resolution request.
  3. IEnumerable resolution and decorator selection now work across parent-child container relations.
  4. I thought a lot about how the resolution from only child containers should be implemented, but I couldn't come up with a solution that would easily fit the current structure. However, as the child containers are now available on the IStashboxContainer interface through the ChildContainers property, it should be fairly easy to construct "child-only" resolution requests with ResolutionBehavior.Current on them. What do you think? Could it be a usable solution?

Could you please take a look at this version and let me know what you think? Thanks!

adambajguz commented 1 year ago

Hi @z4kn4fein,

  1. I like the idea od Chile container identifiers.
  2. Shouldn't this be configurable not to break the existing behavior in case someone is using it (unless you treat is as a bug)?
  3. I think this is ok. When rethought it can alwats be added.
z4kn4fein commented 1 year ago

Hi @adambajguz, thanks for the feedback!

Shouldn't this be configurable not to break the existing behavior in case someone is using it (unless you treat it as a bug)?

Do you mean the IEnumerable/decorator part? If so, I believe the standard behavior should be to consider parent containers, and it was a bug not to do so previously.

Moreover, as it respects the ResolutionBehavior parameter, it's more self-explanatory now as the default is ResolutionBehavior.Defult, which includes parent containers. The old functionality is also available by resolving with ResolutionBehavior.Current.

Of course, I will state in the changelog that the default behavior of these cases changed to let those who may already have built functionality upon this bug know.

I think this is ok. When rethought it can always be added.

Sorry but I don't understand this part, could you please explain what exactly can be always added?

schuettecarsten commented 1 year ago

Is it possible to "extend" child containers with new service registrations? If not, where is the difference between child containers and use of scopes?

Message ID: @.***>

adambajguz commented 1 year ago

@z4kn4fein "Do you mean the IEnumerable/decorator part?" - yes; "it was a bug not to do so previously." - ok

@schuettecarsten "Is it possible to "extend" child containers with new service registrations?" - yes, it is

@z4kn4fein "Sorry but I don't understand this part, could you please explain what exactly can be always added?" ResolutionBehavior.Children can always be added (you removed it because it is not obvious how it should work)

z4kn4fein commented 1 year ago

@adambajguz I've published the latest changes in v5.10.0 with the related extension packages. Please let me know if there are any issues!

@schuettecarsten Yep, as @adambajguz mentioned, it's possible. It's the main point of the child container feature.

adambajguz commented 1 year ago

Hi @z4kn4fein, "When using the StashboxContainer as a DI container for multitenant app that utilizes generic host, it's not possible to easily activate IHostedService from child containers during host startup." - I've tested the new version of Stashbox and there's still a small issue. Imagine having ILogger service in root container and some IHostedService (s) in child container that have a dependecy to ILogger. When using child.ResolveAll<IHostedService>(ResolutionBehavior.Current) the resolution will fail. I think ResolutionBehavior should be extended with AllowDependenciesToResolveFromParent or AllowDependenciesToFallbackToParent flag.

Please let me know if you have a better solution.

adambajguz commented 1 year ago

Or maybe CurrentWithDependencyFallbackToParent = Current | 1 << 2,

z4kn4fein commented 1 year ago

Hey @adambajguz, I've published a new version 5.10.1-preview-811 that contains a new ParentDependency behavior. Current | ParentDependency will allow jumping up to the parent only in the dependency resolution phase. Let me know what you think!

adambajguz commented 1 year ago

Hey @z4kn4fein, thanks! The new ParentDependency flags works fine; so far I don't have any more issues.

z4kn4fein commented 1 year ago

@adambajguz 👍 I've released v5.10.1 with the new extension packages (v5.2.1).