Skip to content

[HTTP] Fix test race with semaphore.#124388

Open
ManickaP wants to merge 1 commit intodotnet:mainfrom
ManickaP:fix/39056-send-timeout-request-content
Open

[HTTP] Fix test race with semaphore.#124388
ManickaP wants to merge 1 commit intodotnet:mainfrom
ManickaP:fix/39056-send-timeout-request-content

Conversation

@ManickaP
Copy link
Member

Fixes #39056

Copilot AI review requested due to automatic review settings February 13, 2026 15:30
@ManickaP
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@ManickaP
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@ManickaP ManickaP marked this pull request as ready for review February 13, 2026 15:31
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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 fixes a race condition in the Send_TimeoutRequestContent_Throws test where the server connection could close before the client properly timed out, causing the test to fail intermittently with an HttpRequestException instead of the expected TaskCanceledException with TimeoutException inner exception.

Changes:

  • Removed the [ActiveIssue] attribute that was tracking the flaky test
  • Added semaphore-based synchronization between client and server to ensure the server stays alive until the client completes its assertions

await server.AcceptConnectionAsync(async connection =>
{
await IgnoreExceptions(connection.ReadRequestDataAsync());
await semaphore.WaitAsync();
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semaphore wait should include a timeout and assertion to prevent the test from hanging if the client-side assertions fail before releasing the semaphore. The established pattern in this test file is to use Assert.True(await semaphore.WaitAsync(5000), "semaphore timed out") to ensure the test fails fast with a clear message rather than hanging indefinitely. See lines 484, 647, and 717 for examples of this pattern.

Suggested change
await semaphore.WaitAsync();
Assert.True(await semaphore.WaitAsync(5000), "semaphore timed out");

Copilot uses AI. Check for mistakes.
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

I wonder if we should make this an option on the factory eventually, e.g. DelayServerShutdownUntilClientCompletion. Not sure it's feasible/worth it to wire though though.
We have such SemaphoreSlim/TaskCompletionSource in a bunch of places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure: System.Net.Http.Functional.Tests.HttpClientTest.Send_TimeoutRequestContent_Throws

2 participants