Fix memory leak in HostFactoryResolver caused by static AsyncLocal#124391
Fix memory leak in HostFactoryResolver caused by static AsyncLocal#124391
Conversation
Co-authored-by: halter73 <[email protected]>
github-actions
bot
added
the
needs-area-label
Co-authored-by: halter73 <[email protected]>
|
|
||
| // We expect most objects to be collected, but allow some to remain due to GC non-determinism | ||
| // Using 80% threshold to reliably catch memory leaks while accounting for GC behavior | ||
| int expectedMinCollected = (weakRefs.Length * 80) / 100; |
There was a problem hiding this comment.
@stephentoub I'm curious if you have any thoughts on this. It's intended to address dotnet/aspnetcore#48047.
I told copilot to add a test similar to ones I wrote for Kestrel like CriticalErrorLoggedIfApplicationDoesntComplete and UnrootedConnectionsGetRemovedFromHeartbeat, but unlike in those cases, we don't have a log to look for.
I can remove the test if we don't think it's helpful. Regardless, I'm tempted to ask tactics to allow backporting this to .NET 8/9/10 even though it's been a long-standing issue. The fix seems very safe, and it's limited to test scenarios.
There was a problem hiding this comment.
It's not clear to me why this change would improve anything? Maybe I'm just missing the point, but the data for an AsyncLocal isn't actually stored in the AsyncLocal, it's stored in a dictionary in the ExecutionContext; the AsyncLocal instance is just a key into that dictionary. Can you elaborate on how this helps?
There was a problem hiding this comment.
I know that AsyncLocals are backed by the ExecutionContext, but I was hoping that there was some ConditionalWeakTable-like behavior where if the AsyncLocal itself became unrooted, it would remove the corresponding entry from the ExecutionContext's AsyncLocals table. Is that not the case? Copilot seemed to think this was the case, but I'm not seeing the code that would do this.
We use WAF quite a bit, and haven't noticed memory leaks ourselves, but the activity on dotnet/aspnetcore#48047 led me to believe where there's smoke there's fire.
I probably should have asked you about this before opening a PR, but I was a bit confused about CCA agent mode vs New Chat mode when I opened this :D
There was a problem hiding this comment.
I was hoping that there was some ConditionalWeakTable-like behavior where if the AsyncLocal itself became unrooted, it would remove the corresponding entry from the ExecutionContext's AsyncLocals table. Is that not the case?
Nope
There was a problem hiding this comment.
Now that I have looked at the AsyncLocal code a bit more, I think the real issue is that people are leaking ExecutionContexts, and making the AsyncLocal non-static only has the potential to make things worse by creating new keys into the dictionary for each instance of the HostingListener.
|
Tagging subscribers to this area: @dotnet/area-extensions-hosting |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a memory leak in HostFactoryResolver caused by a static AsyncLocal<HostingListener> field that prevented garbage collection of HostingListener instances when thread pool threads reused ExecutionContext.
Changes:
- Changed
AsyncLocal<HostingListener>from static to instance field to allow proper garbage collection - Added regression test to verify service providers can be garbage collected after use
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.HostFactoryResolver/src/HostFactoryResolver.cs | Changed _currentListener from static readonly to instance readonly field (line 207) |
| src/libraries/Microsoft.Extensions.HostFactoryResolver/tests/HostFactoryResolverTests.cs | Added NoSpecialEntryPointPattern_DoesNotLeakMemory test that validates 80% of service providers are garbage collected |
halter73
removed
the
needs-area-label
Description
Static
AsyncLocal<HostingListener>field causes memory leak. When thread pool threads reuse ExecutionContext, static AsyncLocal values persist, preventing garbage collection of HostingListener instances and their captured resources.Fix: Make
AsyncLocal<HostingListener>instance-scoped instead of static.Each HostingListener now owns its AsyncLocal, allowing proper cleanup when the listener is disposed.
Customer Impact
Memory accumulates with each host factory resolution, particularly in tooling scenarios (EF migrations, design-time builds) that repeatedly resolve host factories. Long-running processes leak memory on every invocation until eventual OOM.
Regression
No. This is a pre-existing issue present since AsyncLocal was introduced in this code path.
Testing
NoSpecialEntryPointPattern_DoesNotLeakMemorythat verifies 80% of service providers are GC-collected after useRisk
Low. Single-line change with no behavioral impact. The AsyncLocal was only used for event filtering within the same HostingListener instance - making it instance-scoped preserves identical semantics while fixing the leak.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.