-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[H/3] Fix disposing of buffers in parallel with running tasks #117207
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
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 prevents buffers from being returned to the shared pool while offloaded send/read tasks are still running by tracking concurrent operations and moving buffer disposal into those tasks.
- Introduces
_concurrentWriteand_concurrentReadflags - Updates
DisposeSyncHelperto conditionally dispose buffers based on those flags - Adds buffer disposal in the
finallyblocks ofReadResponseAsyncandSendContentAsync - Removes unnecessary
WritesClosedexception observation
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime-libraries-coreclr outerloop |
|
/azp run runtime-libraries stress-http |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
outerloop failures unrelated |
There are 2 cases when we decide to not to wait for HTTP request / response sending / receiving:
runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Line 210 in b8f9b99
runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Line 202 in b8f9b99
As a result,
Http3RequestStream.Disposemight be called in parallel (in case of an error, e.g. cancellation), disposing_recvBuffer/_sendBufferand thus returning their underlying byte array into the shared pool. In the worst case scenario, these tasks could potentially write into byte array thet's been already returned to the pool.So this change, moves the responsibility to dispose the buffers to the non-awaited tasks in case they are send to
LogException. Note that this fixes the parallel usage of the buffer, but might not return the buffers to the pool in all cases (timing dependent). Which seems to me like a reasonable trade-off.Tested by running stress with
-cancelRate 1for an hour, no assert hit (previously it would get hit within few minutes).Also removing observation of
WritesClosedexception which is no necessary since #114226Fixes #93713