Skip to content
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

Avoid race condition in EventSource_ConnectionPoolAtMaxConnections_LogsRequestLeftQueue #52887

Merged
merged 1 commit into from
May 18, 2021

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented May 18, 2021

#50226 added asserts around ActivityIds of events.
It is expecting that IDs of Start/Stop pairs match, relying on the order of events being the same.

The scenario is such that there are 2 requests pending from the client:

  • First has already sent the request and is waiting on the server's response
  • Second is waiting on the queue due to the connection limit being hit (and is therefore blocked on the first one)

The server sends the response to the first request and then processes the second request as fast as possible.

The first request will receive its response and release the second request.
It is at this point that there is a race where the second request could finish and log RequestStop before the first request does.

While this is perfectly fine correctness-wise, the test will fail as it's relying on the order of Starts being the same as that of Stops.

This PR waits before processing the second request until it confirms the first request has finished.

Fixes #52530

@MihaZupan MihaZupan added this to the 6.0.0 milestone May 18, 2021
@MihaZupan MihaZupan requested review from karelz and a team May 18, 2021 06:41
@ghost
Copy link

ghost commented May 18, 2021

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

Issue Details

#50226 added asserts around ActivityIds of events.
It is expecting that IDs of Start/Stop pairs to match, relying on the order of events being the same.

The scenario is such that there are 2 requests pending from the client:

  • First has already sent the request and is waiting on the server's response
  • Second is waiting on the queue due to the connection limit being hit (and is therefore blocked on the first one)

The server sends the response to the first request and then processes the second request as fast as possible.

The first request will receive its response and release the second request.
It is at this point that there is a race where the second request could finish and log RequestStop before the first request does.

While this is perfectly fine correctness-wise, the test will fail as it's relying on the order of Starts being the same as that of Stops.

This PR waits before processing the second request until it confirms the first request has finished.

Fixes #52530

Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http

Milestone: 6.0.0

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan MihaZupan merged commit 610f34e into dotnet:main May 18, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants