autofac / Autofac.Owin

OWIN integration for Autofac
MIT License
23 stars 15 forks source link

Memory Leak Due to ThreadLocalStorage / ConcurrentBag / LifetimeScope Behavior in Owin #25

Closed ivanpfeff closed 3 years ago

ivanpfeff commented 3 years ago

Bug Description

I would love to give a concise description of exactly what this bug is, but I don't fully understand it myself so I will instead try to explain as much of what I know about the issue and hope someone can fill in the gaps.

A web application using the Owin integration has memory leaks if it is under consistent stress which does not allow the request processing threads to exit.

LifetimeScopes are created for each request and are disposed properly when the request ends, but the LifetimeScope references are maintained and as a result garbage collection never cleans up the LifetimeScopes.

Here's a sample reference tree for one such LifetimeScope: image

Rough outline of what happens to cause this issue:

I believe the ConcurrentBag which tracks the ComponentRegistrations is coming from somewhere within the Autofac 6.0 pipeline but I am not sure where.

Steps to Reproduce

I have attached a sample application (see below), this is just a simple Web API .NET Framework application to which I added the Owin pipeline and Autofac Owin integration.

To reproduce, just spam the application (using F5 in a browser works for me) and then take a memory dump. Don't wait too long between spamming and taking the dump or the threads will exit and the GC will clean up the ThreadLocal stuff and eventually clear out the LifetimeScopes. Inside of the memory dump, search for all instances of LifetimeScope and see that there are many hanging around and are all rooted in ConcurrentBags / ThreadLocals.

If you consistently ping the sample site for an extended period of time, you will see the number of LifetimeScopes in the memory dump continue to grow.

Expected Behavior

Under high load, the LifetimeScopes which are created by the Autofac.Owin integration should be cleaned up and released for garbage collection when the request ends.

Dependency Versions

Autofac 6.0 Autofac.Owin 6.0

Additional Info

I was able to work around this problem by specifically removing the OwinContext.Environment key "autofac:OwinLifetimeScope:" which removes the reference to the LifetimeScope and allows it to be garbage collected. This smells like a hack to me. This can be found in NonAutofacMiddleware.cs inside of the sample application.

Sample Application

MemoryLeakSample.zip

If you want to see the results in memory from running requests for an extended amount of time, I wrote a small console app to just repeatedly ping the site:

static void Main(string[] args)
{
    var address = "https://localhost:44387/";
    while (true)
    {
        Console.WriteLine($"Pinging {address} at {DateTime.Now}");
        var client = new HttpClient();
        var response = client.GetAsync(address).ConfigureAwait(false).GetAwaiter().GetResult();
        response.EnsureSuccessStatusCode();

        Thread.Sleep(300);
    }
}
ivanpfeff commented 3 years ago

After some further thought about this, I am not sure that this is an Autofac.Owin issue or an Autofac core issue, it seems that the problematic part is really the use of ConcurrentBag inside of the main Autofac library. Removing the Environment entry for the autofac:OwinLifetimeScope solves the issue of LifetimeScopes not being garbage collected, but it doesn't solve the issue of the ComponentRegistrations not being GCed -- they are much smaller memory-wise but are still leaking.

tillig commented 3 years ago

I haven't been able to totally go through all this, but I see a few interesting things here that may or may not be related.

  1. Use of GlobalConfiguration. OWIN uses a Startup class like ASP.NET Core and does not use GlobalConfiguration at all. We have that documented - one of the most common errors we see is folks trying to use an OWIN pipeline with GlobalConfiguration.
  2. A Startup class in addition to GlobalConfiguration. I do see in the App_Start there's a Startup class (YAY!) but I also see that in Global.asax you're using GlobalConfiguration. That's going to be messy at best.
  3. MVC and Web API in one app with OWIN. This is totally possible to do, but it's hard because MVC doesn't actually run inside the OWIN pipeline. Never has, never will. There is some hackery to get it all to work together, so it'll work, but I don't see any of the MVC OWIN stuff in your app.
  4. No package references to the WebAPI or MVC OWIN integrations. This is a big red flag for me. You can't just add Autofac.Owin and call it good. Both WebAPI and MVC also have additional integration requirements to properly work with OWIN. MVC integration is documented as is WebAPI integration

Here's what I'd recommend:

If, after you've fixed the app to follow the docs and the examples, you still see a memory leak, post a new repro app with all the updates so we can check it out.

ivanpfeff commented 3 years ago

Okay, looking into fixing those things now

ivanpfeff commented 3 years ago

Okay, I'm going to go over this again using the WebAPI OWIN self-host example you linked instead of changing the initial sample application I uploaded so that we're on the same page.

To make the sample serve multiple web requests, I just changed the Program.cs:

using (WebApp.Start<Startup>(baseAddress))
{
    Console.ReadLine();
}

Following that, I did the same thing I mentioned in my initial post -- spam some web requests (against http://localhost:9123/api/test) and then take a memory dump of the application. The request spam was ~10 requests/second and I let it run for roughly 30 minutes.

Here's what I find in the memory dump after letting that run:

image

image

There are 36k LifetimeScopes sitting in memory,

image

They all have GC roots from the ThreadLocal stuff

image

tillig commented 3 years ago

Hmmm. Knowing that GC only runs when there's memory pressure, I'd be curious if a GC.Collect() was run to force the issue if these'd still be around or not.

I'm not honestly personally in a position at the immediate moment to diagnose this. I'm on a Mac and given MVC/WebAPI/OWIN is all Windows it'll require setting up a dev environment for that. I don't know if @alsami or @alistairjevans might have ideas off the cuff. Autofac does use ConcurrentDictionary and ConcurrentBag to help cache things for perf purposes - pipelines for building components and such - but I don't think that should be retained after disposing the child scope.

ivanpfeff commented 3 years ago

Okay, I added a GC.Collect into one of the middlewares after Next.Invoke but was still seeing a build up over time, though less so.

ivanpfeff commented 3 years ago

This is with the GC.Collect call added: image

alsami commented 3 years ago

By taking a quick look at the given screenshots of @ivanpfeff you can see that there is always ConcurrentDictionary<Service, ServiceRegistrationInfo> and ConcurrentBag<IComponentRegistration> high in the ranks of the memory-usage.

As far as I can tell the only place where we create those types is in DefaultRegisteredServicesTracker which does inherit from Disposable. Odd (to me) seems that while each registration is being disposed, the ConcurrentDictionary<Service, ServiceRegistrationInfo> is neither cleared. Though I don't believe that this will necessary be a problem since the garbage-collector at some point should clean that up as well. @alistairjevans do you have an idea?

alistairjevans commented 3 years ago

Each lifetime scope created from BeginLifetimeScope that has custom registrations (in OWIN each request has custom registrations, because it registers IOwinContext per-request), will have its own instance of DefaultRegisteredServicesTracker.

The DefaultRegisteredServicesTracker will be garbage-collected when the lifetime scope is garbage-collected.

In the OWIN integration, the 'owner' of a lifetime scope is the OwinContext for the request; we can see in @ivanpfeff's most recent screenshot that there are the same number of OwinContext objects as there are LifetimeScope objects.

This makes me think the problem is firmly OWIN integration related rather than core Autofac.

@ivanpfeff, can you check you have no code that is holding on to instances of OwinContext somehow?

I suppose it's possible that OWIN is tracking old contexts somewhere, meaning they don't get GC'ed, and neither does the lifetime scope for the request.

There is something we can do to help with this though. The relevant lines in this integration are probably here, where we attach our Autofac scope to the OWIN context. You'll note that we attach the lifetime scope to the OWIN Context, but once we've disposed of the scope, we don't detach it.

I expect the lifetime scopes would get GC'ed much more rapidly if we overwrote the OWIN Context Autofac Item with null. Should be pretty straightforward to prove; @ivanpfeff, did you want to try out such a change and see if it helps?

ivanpfeff commented 3 years ago

Yeah I was yanking the scope out of the OwinContext and that does help with the LifetimeScope garbage collection. That was, for me at least, the majority of the memory issue we were experiencing in our enterprise application, because our LifetimeScopes were quite large due to a large number of per-request dependencies.

I think it would make sense to remove the scope from the OwinContext when your middleware has finished.

As for whether or not I'm holding on to OwinContexts on accident -- I'm using the OWIN self-hosted project from autofac\Examples with only one minor tweak in the Program.cs to make it run, so I have not intentionally added anything to hold references on the OwinContext.

alistairjevans commented 3 years ago

It might simply be that OWIN generally holds onto the context longer than it should do.

Did you want to raise a PR against this integration that clears out that tracking reference to the scope after the middleware exits?

I'm not sure what we can do beyond that really, once the GC cleans up the lifetime scope, everything else should go with it.

ivanpfeff commented 3 years ago

Yeah I will prepare that PR over the weekend, thanks for your input everyone!

ktand commented 3 years ago

Just wanted to chime in and say that we're also experiencing the same issue. Using the configuration below, even without API endpoints, causes leaks. Each request to any route, valid or invalid, leaks a OWinContext.

We see no leaks using Autofac 5.x.

Configuration:

        public void Configuration(IAppBuilder app)
        {
            var config = new HttpConfiguration();
            config.MapHttpAttributeRoutes();

            var builder = new ContainerBuilder();

            var container = builder.Build();
            config.DependencyResolver = new AutofacWebApiDependencyResolver(container);

            app.UseAutofacMiddleware(container);
            app.UseAutofacWebApi(config);
            app.UseWebApi(config);
        }
tillig commented 3 years ago

No need to chime in or +1, if you scroll up just a touch you'll see we already got a PR for the fix merged. We just need to cut a release.

And, of course, if it's not fixed after release, well... we can reopen and continue debugging. If we do have to reopen, we'll likely need some community assistance to pinpoint the problem.

tillig commented 3 years ago

6.0.1 is published and includes this fix and Source Link support for easier debugging.