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

Change SendAsync_SlowServerRespondsAfterDefaultReceiveTimeout_ThrowsHttpRequestException server to Loopback #93025

Merged
merged 4 commits into from
Oct 7, 2023
Merged

Change SendAsync_SlowServerRespondsAfterDefaultReceiveTimeout_ThrowsHttpRequestException server to Loopback #93025

merged 4 commits into from
Oct 7, 2023

Conversation

liveans
Copy link
Member

@liveans liveans commented Oct 4, 2023

Fixes #20675

@ghost ghost assigned liveans Oct 4, 2023
@ghost
Copy link

ghost commented Oct 4, 2023

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

Issue Details

Fixes #20675

Author: liveans
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@liveans liveans requested a review from a team October 4, 2023 19:14
rzikm
rzikm approved these changes Oct 5, 2023
View reviewed changes
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs Show resolved Hide resolved
ManickaP
ManickaP reviewed Oct 5, 2023
View reviewed changes
src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs Outdated
{
Task serverTask = server.AcceptConnectionAsync(async connection =>
{
await connection.SendResponseAsync($"HTTP/1.1 200 OK\r\nDate: {DateTimeOffset.UtcNow:R}\r\nContent-Length: 16000\r\n\r\n");
Copy link
Member

Choose a reason for hiding this comment

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

You're postponing the timeout to the response body portion of HTTP exchange, right? If so, HttpClient.GetAsync will succeed as it by default reads only response headers and doesn't wait for the body.

Copy link
Member

Choose a reason for hiding this comment

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

HttpClient.GetAsync will succeed as it by default reads only response headers and doesn't wait for the body

HttpClient.GetAsync is passing HttpCompletionOption.ResponseContentRead to HttpClient.SendAsync which should enforce reading and buffering the response body. Or am I missing something?

@ManickaP
Copy link
Member

ManickaP commented Oct 5, 2023

Also the affected test is outerloop, so you need to trigger the run manually with /azp run ...

@liveans
Copy link
Member Author

liveans commented Oct 5, 2023

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@liveans
Copy link
Member Author

liveans commented Oct 5, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liveans
Copy link
Member Author

liveans commented Oct 5, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Oct 5, 2023
Closed
@liveans liveans requested a review from ManickaP October 5, 2023 17:59
@build-analysis build-analysis bot mentioned this pull request Oct 5, 2023
Open
@liveans
Copy link
Member Author

liveans commented Oct 6, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liveans
Copy link
Member Author

liveans commented Oct 6, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Oct 6, 2023
Closed
@liveans
Copy link
Member Author

liveans commented Oct 6, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liveans liveans merged commit b21d709 into dotnet:main Oct 7, 2023
174 checks passed
@liveans liveans deleted the sendasync_responds_after_recv_timeout_fix branch October 7, 2023 07:16
@karelz karelz added this to the 9.0.0 milestone Oct 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@antonfirsov antonfirsov antonfirsov approved these changes

@ManickaP ManickaP ManickaP approved these changes

@rzikm rzikm rzikm approved these changes

Assignees

@liveans liveans

Labels
area-System.Net.Http
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

Test failure: SendAsync_SlowServerRespondsAfterDefaultReceiveTimeout_ThrowsHttpRequestException
5 participants
@liveans @ManickaP @antonfirsov @rzikm @karelz