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.

Sign up for GitHub

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

Jump to bottom

Check _abortException before checking _shutdown flag #56552

Merged
merged 2 commits into from
Jul 30, 2021
Merged

Check _abortException before checking _shutdown flag #56552

merged 2 commits into from
Jul 30, 2021

Conversation

alnikola
Copy link
Contributor

@alnikola alnikola commented Jul 29, 2021 •
edited by MihaZupan
Loading

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 in ProcessIncomingFrames 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

@alnikola alnikola requested a review from a team July 29, 2021 15:45
@ghost
Copy link

ghost commented Jul 29, 2021

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

Issue Details

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 in ProcessIncomingFrames 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.

Author: alnikola
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@geoffkizer
Copy link
Contributor

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.

@alnikola
Copy link
Contributor Author

In the test it looks like a race because the test succeeds or failures depending on how the request thread and ProcessIncomingFrames thread are scheduled.

@geoffkizer
Copy link
Contributor

That makes sense, thanks for clarifying.

MihaZupan
MihaZupan approved these changes Jul 29, 2021
View reviewed changes
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!

#56026 is likely the same issue

@alnikola
Copy link
Contributor Author

/azp run runtime-libraries-outerloop

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@alnikola
Copy link
Contributor Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@alnikola
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
Copy link
Member

NameResolution failures are unrelated: #56614

@karelz
Copy link
Member

karelz commented Jul 30, 2021

Given the Outerloop tests in Networking are all accounted for, I think we should be able to merge, right @alnikola?

@alnikola alnikola merged commit facbdce into dotnet:main Jul 30, 2021
@alnikola alnikola deleted the alnikola/1581-fix-invalid-settings-frame branch July 30, 2021 12:21
alnikola added a commit that referenced this pull request Aug 2, 2021
@alnikola
Enable Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCanceled
ececa0b 
The test should have been already fixed by #56552
@alnikola alnikola mentioned this pull request Aug 2, 2021
Merged
alnikola added a commit that referenced this pull request Aug 4, 2021
@alnikola
Enable Http2.SocketSendQueueFull_RequestCanceled_ThrowsOperationCance…
7c8f5fb 
…led (#56723)

The test should have been already fixed by #56552.

Fixes #44352
@karelz karelz added this to the 6.0.0 milestone Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@geoffkizer geoffkizer geoffkizer approved these changes

@MihaZupan MihaZupan MihaZupan approved these changes

Assignees
No one assigned
Labels
area-System.Net.Http
Projects
None yet
Milestone
6.0.0
4 participants
@alnikola @geoffkizer @MihaZupan @karelz