ipjohnson / Grace

Grace is a feature rich dependency injection container library
MIT License
336 stars 33 forks source link

SingletonPerKey disposing per request in Aspnet #256

Closed stephenlautier closed 4 years ago

stephenlautier commented 4 years ago

Hi,

It seems that when registering a service as SingletonPerKey, and the service implements IDisposable in aspnet core, it disposes per request, however instance is created only once.

Execution flow is as follows

This is what i got

scope.Configure(c =>
{
    c.Export<DataClientManager>().As<IDataClientManager>().Lifestyle.SingletonPerKey((scope, context) => "global");
});

public interface IDataClientManager
{
    IDataClientManager Register(object type);
}

public class DataClientManager : IDataClientManager, IDisposable
{
    private readonly HashSet<object> _instances = new HashSet<object>();

    public IDataClientManager Register(object type)
    {
        _instances.Add(type);
        return this;
    }

    public void Dispose()
    {
    }
}

This causes some services such as GQL to not work properly, as they will Dispose and next time round it will blow.

I expects that it only Dispose once the server shuts down in the case of singleton

Versions

I have a working sample as well if needed. Thanks!

ipjohnson commented 4 years ago

I agree it should dispose only at when the host is disposed. Can you email me the sample and I'll take a look at tonight or tomorrow morning.

stephenlautier commented 4 years ago

Its available here https://github.com/sketch7/dotnet.multitenancy/tree/feature/initial-imp (this branch)

Interesting points:

ipjohnson commented 4 years ago

I was able to reproduce this and I'll have some time next week to release a fix. For the next couple days you could try marking the export as externally owned .ExternallyOwned() and it will keep it from being disposed (ever).

ipjohnson commented 4 years ago

Ok I have a fix for it, essentially the wrong disposal context was being used. I've released a new pre-release let me know how it works out for you.

stephenlautier commented 4 years ago

Can you confirm if this one only I need to update Grace: 7.1.1-RC793?

stephenlautier commented 4 years ago

After updating to the mentioned version 7.1.1-RC793, SingletonPerKey as my usage isn't working anymore. Basically can't get the data from .GetExtraData from either scope/context inorder to resolve the tenant

P.S. running the same sample also breaks

ipjohnson commented 4 years ago

Ok I pushed a newer version and tested it with your repo. Usually the scope & disposal scope should be the same so that you don't get data from a child scope but in your case you actually do need the data from the child scope.

stephenlautier commented 4 years ago

Thanks I retested it again, however its partially solved. In my example the IDataClientManager is not being disposed but the service implementing IHeroDataClient e.g. MockHotsHeroDataClient its still being disposed as it was (and is also SingletonPerKey).

Further more I retested the actual scenario (e.g. GQL) and it still happens that the Schema is being disposed

ipjohnson commented 4 years ago

As we’ve talked about this over email and it’s resolved I’m going to close this out. If you have any other issues let me know