Skip to content

Conversation

@ManickaP
Copy link
Member

@ManickaP ManickaP commented Jul 1, 2025

There are 2 cases when we decide to not to wait for HTTP request / response sending / receiving:

As a result, Http3RequestStream.Dispose might be called in parallel (in case of an error, e.g. cancellation), disposing _recvBuffer / _sendBuffer and 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 1 for an hour, no assert hit (previously it would get hit within few minutes).

Also removing observation of WritesClosed exception which is no necessary since #114226

Fixes #93713

@ManickaP ManickaP requested review from a team and Copilot July 1, 2025 17:18
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 1, 2025
Copy link
Contributor

Copilot AI left a 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 _concurrentWrite and _concurrentRead flags
  • Updates DisposeSyncHelper to conditionally dispose buffers based on those flags
  • Adds buffer disposal in the finally blocks of ReadResponseAsync and SendContentAsync
  • Removes unnecessary WritesClosed exception observation

@ManickaP ManickaP added area-System.Net.Http and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 1, 2025
@ManickaP
Copy link
Member Author

ManickaP commented Jul 1, 2025

/azp run runtime-libraries-coreclr outerloop

@ManickaP
Copy link
Member Author

ManickaP commented Jul 1, 2025

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 1, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 1, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 1, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 1, 2025
@ManickaP
Copy link
Member Author

ManickaP commented Jul 2, 2025

outerloop failures unrelated

@ManickaP ManickaP merged commit e4b07f4 into dotnet:main Jul 3, 2025
83 of 87 checks passed
@ManickaP ManickaP deleted the h3-assert branch July 3, 2025 15:25
@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[H/3] Stress run in Debug fails on assert

4 participants