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

Fix HTTP/1.1 concurrent request content read race condition #86715

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

MihaZupan
Copy link
Member

Fixes the HTTP/1.1 part of #53914 (comment)

From reading the code, it looks like it is possible that the request content will be read multiple times concurrently on HTTP 1:

This change fixes that by checking whether allowExpect100ToContinue was already signaled when we encounter an exception and disables retries if so.

@MihaZupan MihaZupan added this to the 8.0.0 milestone May 24, 2023
@MihaZupan MihaZupan requested a review from a team May 24, 2023 17:49
@MihaZupan MihaZupan self-assigned this May 24, 2023
@ghost
Copy link

ghost commented May 24, 2023

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

Issue Details

Fixes the HTTP/1.1 part of #53914 (comment)

From reading the code, it looks like it is possible that the request content will be read multiple times concurrently on HTTP 1:

This change fixes that by checking whether allowExpect100ToContinue was already signaled when we encounter an exception and disables retries if so.

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 8.0.0

@karelz karelz marked this pull request as draft June 1, 2023 13:18
@karelz
Copy link
Member

karelz commented Jun 1, 2023

Moving to Draft for now as we won't have time to work on it for some time.

@ghost ghost closed this Jul 1, 2023
@ghost
Copy link

ghost commented Jul 1, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@MihaZupan MihaZupan reopened this Jul 16, 2023
@MihaZupan MihaZupan marked this pull request as ready for review July 16, 2023 07:56
@MihaZupan
Copy link
Member Author

@dotnet/ncl if someone gets a chance, this one should be pretty easy to review

@MihaZupan
Copy link
Member Author

Thanks

@MihaZupan MihaZupan merged commit d4caa91 into dotnet:main Jul 17, 2023
103 of 105 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 16, 2023
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.

3 participants