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

[release/6.0] add Http3LoopbackConnection.ShutdownAsync and use in AcceptConnectionAsync #58336

Closed
wants to merge 6 commits into from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 30, 2021

Backport of #58088 to release/6.0

Contributes to #57779

Test-only fixes. Should allow re-enabling a bunch of disabled tests (not part of this change).

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.

@danmoseley

/cc @geoffkizer

Customer Impact

Testing

Risk

Low.

@ghost
Copy link

ghost commented Aug 30, 2021

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

Issue Details

Backport of #58088 to release/6.0

/cc @geoffkizer

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@geoffkizer
Copy link
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@karelz karelz added this to the 6.0.0 milestone Aug 31, 2021
@karelz
Copy link
Member

karelz commented Aug 31, 2021

Triage: We want to add re-enabled tests to this PR to improve our ability of servicing 6.0.

@Anipik
Copy link
Contributor

Anipik commented Aug 31, 2021

cc @danmoseley

@danmoseley
Copy link
Member

Approved. Valuable test only changes.

@danmoseley
Copy link
Member

@josalem BasicEventSourceTests.TestsManifestGeneration.Test_EventSource_EtwManifestGenerationRollover hung in coreclr windows x86 .
If you sum all the sleeps (assuming all retries are used) in Test_EventSource_EtwManifestGenerationRollover, they come to over 2 minutes. By default, RemoteExecutor times out after 60 seconds, as here. If you believe progress was happening it just didn't get enough time, you can increase that limit thus:
https://github.com/dotnet/runtime/blob/fc854d9921475366875658180afa495d452451be/src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Unix.cs#L28-L27
My guess is that you expect it to take much less than 60 seconds though...?

@josalem
Copy link
Contributor

josalem commented Aug 31, 2021

My guess is that you expect it to take much less than 60 seconds though...?

I'm not actually familiar with this test. Looking at the failing test code, I would expect this to be making progress, so we can try to queue up a change as you suggested.

@danmoseley
Copy link
Member

so we can try to queue up a change as you suggested.

Sounds good. it will log a complaint if 50% of the timeout is exhausted, which can suggest if the timeout is excessive (if it typically passes without the complaint, then it can probably be reduced). If you crank it up and do'nt see the warning, it can probably go down again some.

@ManickaP
Copy link
Member

ManickaP commented Sep 2, 2021

Triage: We want to add re-enabled tests to this PR to improve our ability of servicing 6.0.

I'm pushing cherry picked changes from these 7.0 issues: is:issue is:closed project:dotnet/runtime/2 label:disabled-test sort:updated-desc milestone:7.0.0

The changes are only re-enabling disabled tests to improve our test coverage in 6.0 branch.

@CarnaViire @wfurt @geoffkizer if I overlooked some PRs (re-enabling tests) that should go with this as well let me know.

@danmoseley does your approval still stand after we've added these tests?

cc @karelz

@ManickaP
Copy link
Member

ManickaP commented Sep 2, 2021

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@karelz
Copy link
Member

karelz commented Sep 2, 2021

Thanks @ManickaP -- FYI @danmoseley this is what we planned to add to the PR before submitting it for approval - see my comment above #58336 (comment).

@karelz
Copy link
Member

karelz commented Sep 2, 2021

Test failures:

    System.Net.Http.Functional.Tests.SyncHttpHandler_HttpClientHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent [FAIL]
      System.TimeoutException : The operation has timed out.
      Stack Trace:
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs(111,0): at System.Net.Http.Functional.Tests.SocketsHttpHandler_Cancellation_Test.Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent()
        --- End of stack trace from previous location ---
    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 ---

Rerunning failed legs.

@karelz
Copy link
Member

karelz commented Sep 6, 2021

Re-run test failures:

    System.Net.Http.Functional.Tests.SocketsHttpHandler_Http2KeepAlivePing_Test.KeepAlivePingDelay_Infinite_NoKeepAlivePingIsSent [FAIL]
      System.TimeoutException : The operation has timed out.
      Stack Trace:
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs(45,0): at System.Net.Http.Functional.Tests.SocketsHttpHandler_Http2KeepAlivePing_Test.KeepAlivePingDelay_Infinite_NoKeepAlivePingIsSent()
        --- End of stack trace from previous location ---
  • 5x Mock System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandler_Finalization_Http3_Mock.IncompleteResponseStream_ResponseDropped_CancelsRequestToServer on Fedora.34, Ubuntu.1804, Ubuntu.1604, CentOS.8, SLES.15 -- same as in previous run
      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 ---
  • 1x MsQuic System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandler_Finalization_Http3_MsQuic.IncompleteResponseStream_ResponseDropped_CancelsRequestToServer on Fedora.34
System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandler_Finalization_Http3_MsQuic.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 ---

Rerunning again

@karelz
Copy link
Member

karelz commented Sep 6, 2021

Re-run no.2 test failures:

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

@karelz
Copy link
Member

karelz commented Sep 7, 2021

Triage: Given the unexpected instability the test-only improvement PR brings on the one test above, we do not think it is worth putting it into 6.0. It would allows us to re-enable only 3 disabled tests, so no super-large value for 6.0.
Closing.

@karelz karelz closed this Sep 7, 2021
@karelz
Copy link
Member

karelz commented Sep 7, 2021

Note: 1 of the failures was actually on MsQuic variant of the test - see #58336 (comment).

@akoeplinger akoeplinger deleted the backport/pr-58088-to-release/6.0 branch September 9, 2021 11:25
@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 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.

7 participants