Dispose is not called on decorated object when using decorator

[andriysavin]

I noticed that when I have IDisposable implemented on a decorator and decoratee, only Dispose of the decorator is called when it goes out of lifetime scope, but the decoratee remains not disposed. I believe this is due to not using service provider for decoratee instance creation.

[khellang]

Yes, that's a limitation of this method. You should probably call Dispose on the decoratee anyway, right? 🤔

[andriysavin]

@khellang Well, strictly saying, since decorator doesn't create and own its decoratee, it should not dispose it. But of course this is a workaround.

[andrewlock]

Hmmm, this does seem like a limitation that should be made very explicit, seems like a good way to get memory leaks otherwise. It's something I hadn't twigged tbh, I assumed the service provider would dispose it when a scope was disposed...

[andriysavin]

@andrewlock because decoratee is created with ActivatorUnitilities and not service provider, the latter knows nothing about the decoratee, so won't dispose it. In my code I managed to implement decoratee creation using service provider, specifically because I wanted dispose guarantees.

[khellang]

That's the only way I was able to prevent a StackOverflowException. I'd love to hear about a better solution 😊

[andriysavin]

@khellang Yeah, I faced the same problem as well :) Take a look at my code here https://andsav.wordpress.com/2018/02/23/decorator-dependency-injection-dotnet-core/.

[andrewlock]

As @andriysavin shows in his post (and after some local tests) the key is using ActivatorUtilities.CreateFactory - that solves the disposal problem! The "nested" service collection approach used in @andriysavin's post isn't a requirement, so I think we should be able to keep the same public API Scrutor has currently. The following is taking @andriysavin's DecoratorServiceCollectionExtensions and tweaking it to use Scrutor's API:

public static class DecoratorServiceCollectionExtensions
{
    public static void Decorate<TService, TDecorator>(
        this IServiceCollection serviceCollection)
        where TDecorator : class, TService
        where TService : class
    {

        var decorateeDescriptors =
            // For now, support defining only single decoratee.
            // TODO: To support cases such as composite decorators 
            // (accepting multiple decoratees and representing them as single)
            // implement handling multiple decoratee configurations.
            serviceCollection.Where(sd => sd.ServiceType == typeof(TService)).ToList();

        if (!decorateeDescriptors.Any())
        {
            throw new InvalidOperationException("No decoratee configured!");
        }

        foreach (var decorateeDescriptor in decorateeDescriptors)
        {
            // We will replace this descriptor with a tweaked one later.
            serviceCollection.Remove(decorateeDescriptor);

            // This factory allows us to pass some dependencies 
            // (the decoratee instance) manually,
            // which is not possible with something like GetRequiredService. 
            var decoratorInstanceFactory = ActivatorUtilities.CreateFactory(
                typeof(TDecorator), new[] { typeof(TService) });

            Type decorateeImplType = decorateeDescriptor.GetImplementationType();

            Func<IServiceProvider, TDecorator> decoratorFactory = sp =>
            {
            // Note that we query the decoratee by it's implementation type,
            // avoiding any ambiguity. 
            var decoratee = sp.GetRequiredService(decorateeImplType);
            // Pass the decoratee manually. All other dependencies are resolved as usual.
            var decorator = (TDecorator)decoratorInstanceFactory(sp, new[] { decoratee });
                return decorator;
            };

            // Decorator inherits decoratee's lifetime.
            var decoratorDescriptor = ServiceDescriptor.Describe(
                typeof(TService),
                decoratorFactory,
                decorateeDescriptor.Lifetime);

            // Re-create the decoratee without original service type (interface).
            // This allows to create decoratee instances via
            // service provider, utilizing its lifetime scope
            // control finctionality.
            var replacement = RefactorDecorateeDescriptor(decorateeDescriptor);

            serviceCollection.Add(replacement);
            serviceCollection.Add(decoratorDescriptor);
        }
    }

    /// <summary>
    /// The goal of this method is to replace the service type (interface)
    /// with the implementation type in any kind of service descriptor.
    /// Actually, we build new service descriptor.
    /// </summary>
    private static ServiceDescriptor RefactorDecorateeDescriptor(ServiceDescriptor decorateeDescriptor)
    {
        var decorateeImplType = decorateeDescriptor.GetImplementationType();

        if (decorateeDescriptor.ImplementationFactory != null)
        {
            decorateeDescriptor =
                ServiceDescriptor.Describe(
                serviceType: decorateeImplType,
                decorateeDescriptor.ImplementationFactory,
                decorateeDescriptor.Lifetime);
        }
        else
        if (decorateeDescriptor.ImplementationInstance != null)
        {
            decorateeDescriptor =
                ServiceDescriptor.Singleton(
                serviceType: decorateeImplType,
                decorateeDescriptor.ImplementationInstance);
        }
        else
        {
            decorateeDescriptor =
                ServiceDescriptor.Describe(
                decorateeImplType, // Yes, use the same type for both.
                decorateeImplType,
                decorateeDescriptor.Lifetime);
        }

        return decorateeDescriptor;
    }

    /// <summary>
    /// Infers the implementation type for any kind of service descriptor
    /// (i.e. even when implementation type is not specified explicitly).
    /// </summary>
    private static Type GetImplementationType(this ServiceDescriptor serviceDescriptor)
    {
        if (serviceDescriptor.ImplementationType != null)
            return serviceDescriptor.ImplementationType;

        if (serviceDescriptor.ImplementationInstance != null)
            return serviceDescriptor.ImplementationInstance.GetType();

        // Get the type from the return type of the factory delegate.
        // Due to covariance, the delegate object can have more concrete type
        // than the factory delegate defines (object).
        if (serviceDescriptor.ImplementationFactory != null)
            return serviceDescriptor.ImplementationFactory.GetType().GenericTypeArguments[1];

        // This should not be possible, but just in case.
        throw new InvalidOperationException("No way to get the decoratee implementation type.");
    }
}

which allows something like the following:

var services = new ServiceCollection();

services.AddScoped<IEmailMessageSender, SmtpEmailMessageSender>());
services.Decorate<IEmailMessageSender, EmailMessageSenderWithRetryDecorator>();

var serviceProvider = services.BuildServiceProvider();

using(var scope = serviceProvider.CreateScope())
{
    var emailSender = serviceProvider.GetRequiredService<IEmailMessageSender>();
    emailSender.SendMessage(new EmailMessage { Body = "Hello, decorator!" });
} // disposes both IEmailSenders

[andrewlock]

@andriysavin I was just looking at this again, and I've just twigged. The reason it disposes correctly is nothing to do with ActivatorUtilities.CreateFactory. It's the fact that you're adding an additional service descriptor for the concrete implementation type, so that you can resolve it from the IServiceProvider.

I believe this is an approach Scrutor could use, but is it too big a change @khellang? Adding the extra descriptor is potentially a breaking change? But having services disposed as people likely expect feels like it could be worth it? I've created a draft PR as an example

[andriysavin]

@andrewlock I didn't say CreateFactory is the way dispose is called (I was mentioning this approach is more efficient as it doesn't use reflection every time). I said that because I use service provider for decoratee creation the former is able to dispose the latter.

[andrewlock]

Yeah sorry, it was my misunderstanding I thought CreateFactory was the magic sauce 🙂

[dotnetjunkie]

I've been researching the possibilities recently, and I like to share my findings with you, and anyone who might find this information interesting.

There are some options in allowing the decorated instances to be disposed, but, as already noted previously, the limitations of the core framework make it hard (if not impossible) to have this implemented properly.

In its essence, what should happen is that the method calling ActivatorUtilities.CreateInstance checks if the returned object is disposable and if so, makes sure the object gets 'tracked' for disposal when the scope ends. This is actually doable to some extend, but proves to be extraordinary difficult to achieve with regards to the following requirements:

1. All disposables created in the context of a scope are disposed in opposite order of creation
2. When the scope is disposed, there must be the guarantee that all Dispose or DisposeAsync methods are called (even if one dispose method throws an exception). Disposal of a scope should continue disposing objects even if one fails.
3. Dispose/DisposeAsync should be guaranteed to be called just once for every tracked component. Even though the dispose pattern states that it should be possible to call Dispose multiple times on the same object without a side effect, it is considered a bad idea for frameworks and libraries to call dispose multiple times on the same object, because it can have performance implications and can confuse developers (it will certainly lead to new 'bug' reports).

These requirements are not theoretical ones; most (if not all) DI Containers (including MS.DI) actually obey to these rules.

Although I found several ways to implement disposable, but none of them adhere to all the previous requirements. I found the possible (partial) solutions:

Using HttpContext.Response.RegisterForDispose

With this solution the HttpResponse's RegisterForDispose method is called from within Scrutor's ServiceCollectionExtensions.CreateInstance extension method.

However, there are a few problems with this approach:

• It requires the existence of an HTTP request, which makes this method unsuited to run on background threads or outside the context of ASP.NET Core (for instance a Console application or even ASP.NET MVC).
• Even within the context of ASP.NET Core, it requires the registration of the IHttpContextAccessor interface, which is probably a bad idea for a library as Scrutor.
• RegisterForDispose will not guarantee opposite order of disposal, because it has its own pipeline, separate from the dispose pipeline of IServiceScope. It will either dispose all its registrations before or after IServiceScope with all its registrations has been disposed.

Add a list of disposables as registration to service scope

This solution might look a bit like this:

private static object CreateInstance(this IServiceProvider provider, Type type, params object[] arguments)
{
    var instance = ActivatorUtilities.CreateInstance(provider, type, arguments);

    // check if object is disposable
    if (instance is IAsyncDisposable || instance is IDisposable)
    {
        // resolve the list and add the disposable to it for later disposal
        provider.GetRequiredService<Disposables>().Add(instance);
    }

    return instance;
}

// list of disposables
private sealed class Disposables : List<object>, IAsyncDisposable
{
    // Implement requirements 1, 2, and 3 here
    public ValueTask DisposeAsync() => // TODO
}

// Add the list of disposables as scoped component to the ServiceCollection
services.AddScoped<Disposables>();

There are, however, a few unfortunate problems with this approach:

• Implementing Disposables to guarantee all three requirements is not an easy task.
• Because Disposables is one single registration, it can not guarantee requirement 1, because all added decorators will be disposed directly after one another. Their dependencies might still be disposed of before they are.
• It's difficult to adhere to requirement 3, because the outermost decorator will be disposed by the service scope as well, which means it will get disposed twice.

The Proper Solution

What's really missing here is an IServiceScope.RegisterForDispose method, because that would hook the disposable to the IServiceScope which could than guarantee that all three requirements are met (IServiceScope does already guarantee those requirements for objects that it has created). Such a method, however, does not exist, and it will be unlikely that it will be added in the future.

So, I think we're in a deadlock here.

[khellang]

I think this issue might've been solved by an ingenious hack by @DanHarltey in #169, which has been pushed to NuGet as part of v4.2.0. It would be cool if you could check it out and report back 😎

[dotnetjunkie]

I'm probably way too late with this comment, but I've been looking at the pull request and am wondering if there isn't an easier way to achieve this (I must admit I don't fully understand the code changes). I noted above #91 (comment) that I thought there was a deadlock with the implementation of disposing decoratees; the PR, of course, proved me wrong.

But wouldn't it be possible to instead add transient disposable wrappers that can be used to allow disposing the undisposed decoratees? Such wrapper could look like this:

public sealed class ScrutorDisposableWrapper : IDisposable, IAsyncDisposable
{
    public object? Disposable;
    public void Dispose() => (this.Disposable as IDisposable)?.Dispose();
    public ValueTask DisposeAsync() => (this.Disposable as IAsyncDisposable)?.DisposeAsync() ?? default;
}

And registered by Scrutor as follows:

services.AddTransient<ScrutorDisposableWrapper>();

This would allow Scrutor, when intercepting the call to the decoratee using a decorator, to -after creating the decoratee-, to resolve a ScrutorDisposableWrapper and assign the decoratee to the ScrutorDisposableWrapper's Disposable field. This would guarantee the following:

1. The resolved decoratee is disposed of in opposite order of creation
2. When the scope is disposed of, the decoratee is guaranteed to be disposed of.
3. Dispose/DisposeAsync is guaranteed to be called just once per decoratee.

When the wrapper is registered as transient, it guarantees each resolve to get a fresh instance, and it allows that fresh instance to be assigned with the -just created- decoratee. When resolving the decoratee from the singleton/container scope, the wrapper can be resolved from the container scope as well. This keeps the wrapper tracked by the container scope and allows the wrapper and its decoratee to get disposed of when the container gets disposed of.

As far as I see, this doesn't require the hack of having this System.Type derivative, which might actually be an unstable solution, because the behavior of how MS.DI checks types might change in the future. I think this could happen because I know that some DI Containers (like Simple Injector) use a different approach to mapping service types to their 'service descriptor'. This allows, for instance, the resolution of proxy types (such as COM objects). This will currently utterly fail when doing this with MS.DI, but once Microsoft starts fixing this limitation in the future... this DecoratedType hack might very well break. The hack might actually already be a breaking change for some of DI Containers that do this kind of type checking. We might argue of course that those DI Container's adapters break the MS.DI adapter contract (the Conforming Container), but that's arguable, because the contract is certainly unclear about this.

I hope this helps.

[khellang]

Wrapping anything in transient disposables would probably lead to a few issues as well, no?

[dotnetjunkie]

I can't think of any. But I could certainly be missing something. What issues are you thinking of?

[khellang]

Resolving transient disposables from the root container creates a memory leak and won't dispose until application shutdown?

[khellang]

aspnet/DependencyInjection#456

[dotnetjunkie]

Resolving transient disposables from the root container creates a memory leak and won't dispose until application shutdown?

But that's exactly.what you want to happen when you created a singleton decoratee; it should be disposed once at application shutdown. So as long as you create one wrapper for one created singleton decoratee, there's no memory leak, just one extra onject in memory to allow disposing of that singleton.

If, however, you create a new wrapper instance everytime you resolve that same instance... well than you'll be f... for sure in a place where you have a memory leak. So this is what should be prevented.

[khellang]

But that's exactly.what you want to happen when you created a singleton decoratee

Right, but this feature needs to support more than singletons 😅

[dotnetjunkie]

Right, but this feature needs to support more than singletons

Absolutely. But the trick here is to ensure that the wrapper is resolved from the same scope the decoratee would be resolved of. That would ensure the wrapper lives as long as the decoratee is intended to live and it gets disposed at the right time and in the right order.

[khellang]

But the trick here is to ensure that the wrapper is resolved from the same scope the decoratee would be resolved of.

But how do I control that? There's no way of knowing the scope during resolution (or registration). I think you must always assume a registration can be resolved from any scope. This is what makes disposable transients so dangerous and why they should've never been tracked for disposal in the first place.

[dotnetjunkie]

But how do I control that?

Currently, (or at least in the v4.0 version of Scrutor that I looked at) decorators are always applied using the same lifetime as their decoratee, which means they are always resolved using the same scope. This would mean that a simple adjustment of the private static ServiceDescriptor Decorate(this ServiceDescriptor descriptor, Type decoratorType) to apply tracking would probably suffice, for instance:

private static ServiceDescriptor Decorate(this ServiceDescriptor descriptor, Type decoratorType)
{
    return descriptor.WithFactory(
        provider => provider.CreateInstance(
            decoratorType, 
            provider.TrackInstance( // <-- This call is what I added
                provider.GetInstance(descriptor))));
}

private static object TrackInstance(this IServiceProvider provider, object instance)
{
    if (instance is IAsyncDisposable)
    {
        // As this wrapper is resolved before the decorator's factory method returned, MS.DI
        // ensures the wrapper is disposed -after- the decorator. In other words: it's disposed
        // in correct opposite order of creation.
        provider.GetRequiredService<ScrutorDisposableWrapper>().Disposable = instance;
    }
    else if (instance is IDisposable)
    {
        // Compared to what I said above, you likely need two wrapper classes. This one *only*
        // implements IDisposable. Resolving an IAsyncDisposable in a scope that is disposed using
        // .Dispose() compared to calling DisposeAsync causes an exception, because
        // IAsyncDisposables can't be disposed of using Dispose.
        provider.GetRequiredService<ScrutorNonAsyncDisposableWrapper>().Disposable = instance;
    }

    return instance;
}

The call to TrackInstance would cause problems (such as memory leaks or breaking the expectations) in case it would get called multiple times on the same instance, but the fact that the decorator and decoratee have the same lifestyle prevents this, because the decoratee is cached by MS.DI using the provided lifetime.

I must admit that I'm not familiar enough with the Scrutor code base, so it's possible that you might need to add this tracking code in multiple places, but I'll let you be the judge of that.

Another complication might be the use of IAsyncDisposable. I noticed that -the v4 version at least- doesn't have a dependency on IAsyncDisposable and I'm unsure whether it would be a problem for you to require such dependency on your library. There are ways around requiring a hard dependency on Microsoft.Bcl.AsyncInterfaces, but this requires a fair amount of duck typing. If you want to know more, I wrote about how I did this with Simple Injector here.

[DanHarltey]

Hi @dotnetjunkie ,

Thank you for the feedback on the method and your suggestions.

I do like the method of keeping the services within the DI containers like we added. I also envisioned it would allow decorators to have different lifestyles than of the original service i.e. #38.

However, I do understand your concerns around stability and the numerous different DI containers.

As for the Type object changing, whilst it is a big object it is designed to be inherited, and pretty stable only seen a few modifications to .Nets existence.

I think I can see the ScrutorDisposableWrapper idea working. I think you would and up needing something with a collection for singletons. But would have to play around with the code a bit to understand it fully.

[dotnetjunkie]

@khellang, @DanHarltey, I'm certainly not pushing my proposal; just wanted to give my two cents here and a possible alternative approach.

I'm also unsure what the consequences would be when you start to differentiate decorator lifestyles (@DanHarltey, nice that you are referring to #38, a discussion that I started). This will certainly complicate things, but I assume it is detectable what scope a decoratee is resolved from. Although, while typing this, I realize that we're talking about MS.DI, which was of course not designed for this kind of extensibility. As I said before, I think it's quite amazing what Scrutor managed to do on top of MS.DI.

But this does mean that a proposal such as mine must be thoroughly placed with and tested to fully understand its implications. Whether this is worth the trouble... that's of course up to you.