-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Dev/rokonec/101022 httplistner mem leak #114098
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
Dev/rokonec/101022 httplistner mem leak #114098
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a memory leak issue caused by HttpListenerContext not being released from the tracking dictionary by wrapping the WebSocket in a new disposable class.
- Introduces HttpListenerWrappedWebSocket to ensure connections are properly closed.
- Modifies instantiation to use the wrapped WebSocket for cleanup on Dispose.
Comments suppressed due to low confidence (1)
src/libraries/System.Net.HttpListener/src/System/Net/Managed/WebSockets/HttpWebSocket.Managed.cs:121
- Consider wrapping the _context.Response.Close() call in a try/catch block within Dispose to protect against potential exceptions that might disrupt the disposal process.
public override void Dispose()
|
Tagging subscribers to this area: @dotnet/ncl |
Is it possible to translate your repro into a test? Or at least try to design a test to repro it, to get some coverage for this. |
I thought about it and have not found how to do it. I was also looking for similar tests and there are none 🤦 Update: After some discussion we decided to not unit test this memory leak as this is complicated and expensive to run. |
ManickaP
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
cc @CarnaViire
|
Does IMO not disposing the |
Windows implementation is different, it does not instantiate standard Edit: After internal discussion we decide to implement cleaning in finalizer, to handle situation when users forget are not disposing websocket which in Windows would not cause memory leak but after migration to linux, it would. |
916466d to
8ee17bf
Compare
Fixes #101022
Context
Unix only.
HttpListenerContextis not released fromDictionary<HttpListenerContext, HttpListenerContext> _listenerContextswhen code is written by best and documented practices. Cleaning is done by disposing websocket instance but websocket does not close connection, which is needed to properly cleanup.This is causing HttpListenerContext piling in
_listenerContextsresulting in memory leak.See #101022 for more details.
Code changes
I have not found other solution than wrapping websocket into private class
HttpListenerWrappedWebSocketalong with needed objects to cleanup connection at websocket Dispose.Testing
I have verified that my mini repro (client and sever consoles) no longer reproes after give changes been compiled into experimental dotnet Runtime.