-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Check _abortException before checking _shutdown flag #56552
Check _abortException before checking _shutdown flag #56552
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsIn case of an error aborting the connection, there is a race between a thread creating new This PR adds
|
To be clear, it's not a race, right? We are taking the lock in both places. The issue is just that we aren't checking for _abortException in AddStream, so we are always treating the request as retryable. |
In the test it looks like a race because the test succeeds or failures depending on how the request thread and |
That makes sense, thanks for clarifying. |
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.
Thanks!
#56026 is likely the same issue
/azp run runtime-libraries-outerloop |
No pipelines are associated with this pull request. |
/azp list |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
NameResolution failures are unrelated: #56614 |
Given the Outerloop tests in Networking are all accounted for, I think we should be able to merge, right @alnikola? |
The test should have been already fixed by #56552
In case of an error aborting the connection, there is a race between a thread creating new
Http2Stream
to send a request and the thread looping inProcessIncomingFrames
that sets _shutdown flag and_abortException
. If the request thread first sees_shutdown == true
, then it won't see the_abortException
even if it's set, so the request will be retried when it shouldn't.This PR adds
_abortException
check just before the_shutdown == true
check to make sure an abort exception is observed.Fixes #1581
Fixes #56138
Fixes #56026