Mvc: Storing tasks for a long time can hold onto the ExecutionContext which can leak memory (like the HttpContext)

[davidfowl]

MVC has a couple of places where it stores Tasks in the MemoryCache or a dictionary for a long period of time. That can result in capturing the HttpContext and other ambient async locals which may result in a memory leak (see https://github.com/dotnet/coreclr/issues/20273).

The potential leak exists here:

• https://github.com/aspnet/Mvc/blob/b156dee4f175406a7cdcaaeb2b2a393f2a6abca0/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs#L116
• https://github.com/aspnet/Mvc/blob/07cc9e66c6d936cbf5f8ddf1d37fa625ab3fb365/src/Microsoft.AspNetCore.Mvc.Razor/Internal/RazorViewCompiler.cs#L196

cc @pranavkm

[Lakritzator]

Just out of curiosity, while reading into this issue, I saw a TaskCompletionSource which was not created with TaskCreationOptions.RunContinuationsAsynchronously as was advised recently here.

e.g.:
https://github.com/aspnet/Mvc/blob/b156dee4f175406a7cdcaaeb2b2a393f2a6abca0/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs#L111

I know this is not always needed, and you probably know your code better than me, just want to make sure this is not overlooked...

The List of potential issues: https://github.com/aspnet/Mvc/search?q=%22new%20TaskCompletionSource%22

[davidfowl]

It's probably needed 😄

[mkArtakMSFT]

Thanks folks.

@pranavkm, this is all yours!

[pranavkm]

The Task created by TaskCompletionSource<> does not capture context, so the cached task in RazorViewCompiler is OK. CacheTagHelper caches the result of this method: https://github.com/aspnet/Mvc/blob/release/2.2/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs#L218-L227, which effectively produces a Task.FromResult(new CharBufferHtmlContent(writer.Buffer)). No context is captured in this case either.

Specifying the TaskCreationOptions seems to be the only work to do here.

[rynowak]

Are you sure about that? /cc @stephentoub

I did some wacky things in routing to try and avoid this issue: https://github.com/aspnet/Routing/blob/release/2.2/src/Microsoft.AspNetCore.Routing/EndpointRoutingMiddleware.cs#L122

[pranavkm]

Hmm, I could have another look, but I tried looking at the CapturedContext on the Task (with and without debugging) and it consistently turned up as null. @davidfowl recommended injecting an IHttpContextAccessor since that seems to be a common theme with these kinds of memory leak and I did not see any values being captured in the context.

[stephentoub]

The Task created by TaskCompletionSource<> does not capture context

Are you sure about that?

Tasks created by TaskCompletionSource do not capture ExecutionContext: it's only captured when there's actually something to execute by the task, which is not the case when it's used purely as a promise, as it is with TCS.

And for other cases (e.g. Task.Run) we also now clear out ExecutionContext even when it was captured, so there shouldn't be any issues here moving forward.
https://github.com/dotnet/coreclr/pull/20294

[rynowak]

OK DOPE - I can probably simplify the code in routing then.