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

Possible memory leak when using HttpRequestMessage tracking #628

Closed
siimv opened this Issue Oct 31, 2018 · 9 comments

Comments

2 participants
@siimv

siimv commented Oct 31, 2018

When using HttpRequestMessageTracking in WebAPI 2, there is a possible memory leak. When running memory profiler, I see a lot of HttpRequestMessageWrapper classes in memory which are not garbage collected.
I'm using latest version of the packages (4.3.0), application is WebAPI 2 project targeting .NET 4.6.2 (no .Net Core).

Now the interesting part - I'm not able to reproduce it on machines where .NET 4.7.2 is installed but can reproduce it constantly on machines which have only .NET 4.6.2. I had similar issue with the version 4.0.12, but then I was able to reproduce it also on machines with .NET 4.7.2 and decided to upgrade to 4.3.0.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 31, 2018

Ouch! It seems like Microsoft fixed a bug concerning System.Runtime.Remoting.Messaging.CallContext (which is what Simple Injector uses under the cover to cache HttpRequestMessage instances) in .NET 4.7.

I pushed a test version (v4.3.1-alpha1) of the SimpleInjector.Integration.WebApi package to NuGet. In this version, Simple Injector's internal SimpleInjectorHttpRequestMessageHandler clears the cached HttpRequestMessage at the end of the request.

Would you be willing, and able, to try out test test version to see whether or not:

  1. would fix the memory leak in 4.6?
  2. it breaks your application?
@siimv

This comment has been minimized.

siimv commented Nov 1, 2018

I tested the new alpha version. Unfortunately results are same - didn't fix memory leak under 4.6.
But continued to work with 4.7 and seemingly didn't break anything.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Nov 1, 2018

Hmmm.... that's weird. Is the amount of leaked memory the same? I expected that fix the problem. Would you be able to try replacing the integration code with the following:

using System;
using System.Net.Http;
using System.Runtime.Remoting.Messaging;
using System.Threading;
using System.Threading.Tasks;

internal sealed class AmbientHttpRequestMessageHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
        CancellationToken cancellationToken)
    {
        HttpRequestMessageProvider.CurrentMessage = request;

        try
        {
            return await base.SendAsync(request, cancellationToken);
        }
        finally
        {
            HttpRequestMessageProvider.CurrentMessage = null;
        }
    }
}


internal static class HttpRequestMessageProvider
{
    private static readonly string Key = Guid.NewGuid().ToString("N").Substring(0, 12);

    internal static HttpRequestMessage CurrentMessage
    {
        get
        {
            var wrapper = (HttpRequestMessageWrapper)CallContext.LogicalGetData(Key);

            return wrapper != null ? wrapper.Message : null;
        }

        set
        {
            var wrapper = value == null ? null : new HttpRequestMessageWrapper(value);

            CallContext.LogicalSetData(Key, wrapper);
        }
    }

    [Serializable]
    internal sealed class HttpRequestMessageWrapper : MarshalByRefObject
    {
        [NonSerializedAttribute]
        internal readonly HttpRequestMessage Message;

        internal HttpRequestMessageWrapper(HttpRequestMessage message)
        {
            this.Message = message;
        }
    }
}

Add the AmbientHttpRequestMessageHandler to Web API as follows:

// This replaces the call to container.EnableHttpRequestMessageTracking(config)
config.MessageHandlers.Insert(0, new AmbientHttpRequestMessageHandler());

And get the HttpRequestMessage as follows:

// This replaces the call to container.GetCurrentHttpRequestMessage()
var message = HttpRequestMessageProvider.Current;

The supplied AmbientHttpRequestMessageHandler sets the HttpRequestMessageProvider.Current at the start of the request, and clears it at the end of the request. If there is a memory leak in CallContext, this should clear it out.

@siimv

This comment has been minimized.

siimv commented Nov 1, 2018

I may be wrong - after examining memory dump, I don't see HttpRequestMessageWrapper objects any more, but TimerQueueTimer objects (they were previously also present, but overwhelmed by HttpRequestMessageWrapper).

So my guess is there should be some more similar issues - either with the packages or with application code itself.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Nov 1, 2018

Ahh, great. So that probably fixes the memory problems caused by Simple Injector (in combination with a bug in .NET). Simple Injector does not make use of anything related to TimerQueueTimer, that seems to be an async thingy. Is this related?

@siimv

This comment has been minimized.

siimv commented Nov 1, 2018

Yeap, I figured it's related to async. Now I have to figure out the exact cause.

Bus seems Simple Injector related issues are resolved with this fix, thanks :)

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Nov 1, 2018

I will add the bugfix to v4.4, but will not be releasing an RTM version of 4.3.1. You can keep using the 4.3.1-alpha1 package for the time being; the only change compared to 4.3.0 is this fix.

btw, thank you for taking the time to report this, and trying out the alpha release. Much appreciated.

@dotnetjunkie dotnetjunkie added this to the v4.4 milestone Nov 1, 2018

@siimv

This comment has been minimized.

siimv commented Nov 1, 2018

FYI, I was able to track down my problem. And seems it's similar to the issue you referred.

But the route cause was application code, singleton factory wasn't returning singleton objects (it created new instance each time - which used HttpRequestMessage and HttpClient connections) and it seems it was the reason those two other bugs occurred:

  • Simple Injector bug
  • and similar bug like this
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Nov 2, 2018

A new version of Simple Injector (v4.4) has been released that contains this feature/fix.

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