AutofacWebApiDependencyScope never disposed and stays in finalizer queue

[srogovtsev]

Describe the Bug

We seem to have problem with AutofacWebApiDependencyScope sticking in the finalizer queue and consuming resources.

Steps to Reproduce

1. Wire Autofac as recommended:

app
.UseAutofacWebApi(configuration)
.UseWebApi(configuration);

2. Add something big to the scope (in our case it was a property in HttpContext, which was stored in IOwinContext, which was stored in ILifetimeScope
3. Observe memory consumption after the request has ended: in our case we have AutofacWebApiDependencyScope in the finalizer queue, undisposed, holding a reference to disposed ILifetimeScope, which holds a reference to no longer relevant resources

Expected Behavior

AutofacWebApiDependencyScope doesn't stay in memory after the request is complete

Additional Info

It is my understanding of DependencyScopeHandler code that it, in fact, creates a AutofacWebApiDependencyScope, which is never disposed of, and as such will leave the AutofacWebApiDependencyScope for the finalizer to consume:

Autofac.WebApi.Owin/src/Autofac.Integration.WebApi.Owin/DependencyScopeHandler.cs

Lines 47 to 50 in aa10f6f

	var dependencyScope = new AutofacWebApiDependencyScope(lifetimeScope);
	request.Properties[HttpPropertyKeys.DependencyScope] = dependencyScope;
	return base.SendAsync(request, cancellationToken);

This can be pretty easily rectified by adding the scope to either ILifetimeScope.Disposer or HttpRequestMessage.AddResourceForDisposal, but I assume I am missing some rationale for why this was originally done like this.

[srogovtsev]

Actually, even simpler solution would be just to suppress finalization for created AutofacWebApiDependencyScope, because it doesn't have its own resources, and we expect outer OWIN pipeline to properly dispose of the ILifetimeScope.

[tillig]

I remember there were some challenges around the timing of creation/disposal of the various lifetime scope incarnations (the actual request lifetime scope, the AutofacWebApiDependencyScope, etc.) and getting them to align well when folks tried to use MVC, Web API, OWIN, and even sometimes Web Forms, all in the same application. "We let X dispose the scope, but Y is still alive and needs it!" Lots of pain around that, since OWIN was sort of a bolt-on after the fact.

I don't remember if that's why we left disposal like this or if there was something else.

I do see that Web API already appears to inherently register the dependency scope for disposal if it's the thing that initially creates it (which is what would happen in the non-OWIN scenario). In the OWIN scenario, we need to make sure we at least let the Autofac ILifetimeScope live until it gets back to the OWIN middleware - if we actively dispose AutofacWebApiDependencyScope with the request message, I don't know how that impacts middleware that would need the nested ILifetimeScope after the WebAPI portion of things runs.

However, I think that latter solution - GC.SuppressFinalize(dependencyScope) - is probably a reasonable solution to the issue you're seeing. It would mean the OWIN pipeline still gets to dispose the ILifetimeScope (rather than having the AutofacWebApiDependencyScope do it) but you wouldn't end up seeing it in the finalizer queue.

Unclear why this never came up before, but given this stuff is many years old, it could be anything from "the finalization mechanism has changed and it wasn't a problem before" to "no one really cared that many years ago and it's only now that it's been interesting to solve."

[srogovtsev]

Would you accept a patch with GC.SuppressFinalize?

…

On Fri, 15 Jul 2022 at 10:08, Travis Illig ***@***.***> wrote: I remember there were some challenges around the timing of creation/disposal of the various lifetime scope incarnations (the actual request lifetime scope, the AutofacWebApiDependencyScope, etc.) and getting them to align well when folks tried to use MVC, Web API, OWIN, and even sometimes Web Forms, all in the same application. "We let X dispose the scope, but Y is still alive and needs it!" Lots of pain around that, since OWIN was sort of a bolt-on after the fact. I don't remember if that's why we left disposal like this or if there was something else. I do see that Web API already appears to inherently register the dependency scope for disposal <https://github.com/jbogard/aspnetwebstack/blob/730c683da2458430d36e3e360aba68932ba69fa4/src/System.Web.Http/HttpRequestMessageExtensions.cs#L57> if it's the thing that initially creates it (which is what would happen in the non-OWIN scenario). In the OWIN scenario, we need to make sure we at least let the Autofac ILifetimeScope live until it gets back to the OWIN middleware <https://github.com/autofac/Autofac.Owin/blob/04476d90a2d7a73782146f5d840218861dfe37ff/src/Autofac.Integration.Owin/AutofacAppBuilderExtensions.cs#L393> - if we actively dispose AutofacWebApiDependencyScope with the request message, I don't know how that impacts middleware that would need the nested ILifetimeScope after the WebAPI portion of things runs. However, I think that latter solution - GC.SuppressFinalize(dependencyScope) - is probably a reasonable solution to the issue you're seeing. It would mean the OWIN pipeline still gets to dispose the ILifetimeScope (rather than having the AutofacWebApiDependencyScope do it) but you wouldn't end up seeing it in the finalizer queue. Unclear why this never came up before, but given this stuff is many years old, it could be anything from "the finalization mechanism has changed and it wasn't a problem before" to "no one really cared that many years ago and it's only now that it's been interesting to solve." — Reply to this email directly, view it on GitHub <#15 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARZOZKLFADHHAUHBYZ3BBLVUFWG7ANCNFSM53VN5PKQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[tillig]

Sure.

I will say, though, that it might take a bit to get an updated release cut. As we're touching these older packages, we're taking the time to get all the build tooling and such up to date, start publishing .snupkg symbols, etc. I can totally handle all that, I just wanted to set expectations that once the PR is accepted, you won't see a release in the next 10 minutes or something. It'd be next week.

[srogovtsev]

you won't see a release in the next 10 minutes or something. It'd be next week.

Oh, this is perfectly fine (thank for the notice anyway), frankly, I wasn't expecting any answer till next week. If anything, I appreciate that you keep even older packages up to recent standards.

And to elaborate on your earlier comment, as I've finally back to regular keyboard:

I remember there were some challenges around the timing of creation/disposal of the various lifetime scope incarnations (the actual request lifetime scope, the AutofacWebApiDependencyScope, etc.) and getting them to align well when folks tried to use MVC, Web API, OWIN, and even sometimes Web Forms, all in the same application.

Yep, that's me exactly, I do remember those challenges vividly (and I think maybe even an occasional PR or two related to the reuse of the lifetime scopes), so I do appreciate the complexity.

In the OWIN scenario, we need to make sure we at least let the Autofac ILifetimeScope live until it gets back to the OWIN middleware

Yeah, this is why my first proposal was to tie created AutofacWebApiDependencyScope to the ILifetimeScope.Disposer, which would dispose the (dependency) scope with the (lifetime) scope, which is definitely when nobody else would be using it anyways (if they tried, they'd just get "scope is disposed" from ILifetimeScope anyways).

But after a consideration, just suppressing the GC altogether seems to be a cleaner way of achieving everyone's happiness without introducing too much stuff.

Unclear why this never came up before, but given this stuff is many years old, it could be anything from "the finalization mechanism has changed and it wasn't a problem before" to "no one really cared that many years ago and it's only now that it's been interesting to solve."

My take is that nobody really cared, because usually there isn't that much stuff in the lifetime scope. All the really dangerous stuff is usually under IDisposable, which ILifetimeScope perfectly cares for, so a slight accumulation of references till the next finalizer run is not a big deal. It's just our particular case, when we have some state (that we, frankly, shouldn't have had in the first place, but that's an altogether different architectural challenge of ours) that is referenced by the scope indirectly, which is why it cannot be properly disposed, and in a particular case something big hit that state, so we were investigating memory dump and came onto this. I'm still not even convinced this is a recurring issue, but it'd be nice to plug it.

Anyways, I'll be drafting the PR this weekend. The only thing that bothers me is that I don't really understand if I can write a test for this scenario.

[srogovtsev]

It has just occurred to me that the finalizer for AutofacWebApiDependencyScope actually does nothing (it calls Dispose with false, which does nothing). As can be seen in MS documentation (https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#implement-the-dispose-pattern, see the first example, BaseClassWithSafeHandle), we can just remove the finalizer altogether. Shouldn't we do that instead of suppressing the finalization?

[srogovtsev]

In fact, according to https://stackoverflow.com/a/29150251/1105881, GC.SuppressFinalize would not be enough, as it will remove the object from the "freachable queue", but not the finalization queue, so this probably raises the importance of completely removing the finalizer.

[tillig]

I'm going to leave this open just for tracking purposes - the whole solution requires that we also take the updated Autofac.WebApi2 package, which I'm working on releasing.