Skip to content

Conversation

@geoffkizer
Copy link
Contributor

Contributes to #57779

Add a ShutdownAsync method on the HTTP3 loopback connection. This will send a GOAWAY frame to the client and wait for the client to close the connection gracefully. It will also eat connection abort exceptions if the client already has closed the connection.

Use this method in AcceptConnectionAsync, after the user's connection handling code has finished. The intent here is to avoid the kind of data loss seen in #57779. and avoid HTTP3-specific one-offs to reenable tests as in #58041.

Also, some fixes to GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly to work properly with these changes and clean up some unnecessarily complicated logic.

@ManickaP @CarnaViire

@ghost ghost added the area-System.Net.Http label Aug 25, 2021
@geoffkizer geoffkizer added this to the 6.0.0 milestone Aug 25, 2021
@ghost
Copy link

ghost commented Aug 25, 2021

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

Issue Details

Contributes to #57779

Add a ShutdownAsync method on the HTTP3 loopback connection. This will send a GOAWAY frame to the client and wait for the client to close the connection gracefully. It will also eat connection abort exceptions if the client already has closed the connection.

Use this method in AcceptConnectionAsync, after the user's connection handling code has finished. The intent here is to avoid the kind of data loss seen in #57779. and avoid HTTP3-specific one-offs to reenable tests as in #58041.

Also, some fixes to GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly to work properly with these changes and clean up some unnecessarily complicated logic.

@ManickaP @CarnaViire

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member

I ran this with the re-enabled test from #58041, it works nicely. I'd merge this and than I can merge my PR with just re-enabling the test.

@ManickaP
Copy link
Member

ManickaP commented Aug 25, 2021

We have some failing mock tests:

System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandler_Finalization_Http3_Mock.IncompleteResponseStream_ResponseDropped_CancelsRequestToServer [FAIL]
      System.TimeoutException : The operation has timed out.
      Stack Trace:
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs(33,0): at System.Net.Http.Functional.Tests.HttpClientHandler_Finalization_Test.IncompleteResponseStream_ResponseDropped_CancelsRequestToServer()
        --- End of stack trace from previous location ---

Seems relevant.

EDIT: reproduces locally on the current main with this change.

@geoffkizer
Copy link
Contributor Author

The failure in IncompleteResponseStream_ResponseDropped_CancelsRequestToServer seems to be caused by this: #58234

Since this test is not actually working as intended for HTTP3, I am going to disable it once again.

@geoffkizer geoffkizer force-pushed the shutdownloopbackconnection branch from 708ae59 to f553d82 Compare August 27, 2021 03:01
@geoffkizer
Copy link
Contributor Author

I've pushed some changes that hopefully resolve all the issues.

(1) Disable IncompleteResponseStream_ResponseDropped_CancelsRequestToServer again because it's actually testing what we want it to, and because this change causes it to fail again. More info: #58234
(2) Simplify the change to GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly, which should fix the WinHttpHandler failure and minimize possible disruption here in general.

Please take a look.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer
Copy link
Contributor Author

Even though I reverted the more complicated changes to GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly, I think these are still worth doing in the long run.

Tracked here: #58238

@ManickaP
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP merged commit 46f4d95 into dotnet:main Aug 27, 2021
@geoffkizer
Copy link
Contributor Author

@karelz @ManickaP I believe we want to backport for RC2, right?

@geoffkizer
Copy link
Contributor Author

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1180628641

@geoffkizer geoffkizer added this to the 7.0.0 milestone Aug 31, 2021
@karelz karelz modified the milestones: 6.0.0, 7.0.0 Aug 31, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2021
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