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

Ignore LoopbackServer exceptions in MaxHeadersLength test #73937

Closed
wants to merge 6 commits into from

Conversation

MihaZupan
Copy link
Member

Fixes #73930

@MihaZupan MihaZupan added this to the 7.0.0 milestone Aug 15, 2022
@MihaZupan MihaZupan requested review from karelz and ManickaP August 15, 2022 10:52
@ghost
Copy link

ghost commented Aug 15, 2022

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

Issue Details

Fixes #73930

Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http

Milestone: 7.0.0

ManickaP
ManickaP previously approved these changes Aug 15, 2022
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Small suggestion.

@MihaZupan
Copy link
Member Author

MihaZupan commented Aug 15, 2022

The test is failing with "inactivity" on both client and server side (console logs):

System.Net.Quic.QuicException: The connection timed out from inactivity.

It also hit an Assert with no message that I can't find anywhere near the reported line in Http3Connection.

I don't see a reason why both sides would be hanging on this specific test.
It seems like it could be a product issue (or at least an issue with the loopback server).
cc: @ManickaP @CarnaViire @rzikm

I will disable this specific test for H3 for now instead to clean up CI.


Even this fails:
https://github.com/dotnet/runtime/blob/2a2f6732fc86c7665fb317d60fe178aa3b619200/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.MaxResponseHeadersLength.cs#L52-L61

@MihaZupan MihaZupan added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 15, 2022
@ManickaP
Copy link
Member

ManickaP commented Aug 15, 2022

Based on the console logs, in both cases the server is waiting on data on the control stream:

at System.Net.Quic.ResettableValueTaskSource.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token) in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs:line 267
at System.Net.Quic.QuicStream.ReadAsync(Memory`1 buffer, CancellationToken cancellationToken) in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs:line 287
at System.Net.Test.Common.Http3LoopbackStream.ReadIntegerAsync() in /_/src/libraries/Common/tests/System/Net/Http/Http3LoopbackStream.cs:line 462
at System.Net.Test.Common.Http3LoopbackConnection.<EnsureControlStreamAcceptedAsync>g__EnsureControlStreamAcceptedInternalAsync|39_0() in /_/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs:line 149
at System.Net.Test.Common.Http3LoopbackConnection.AcceptRequestStreamAsync() in /_/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs:line 165
at System.Net.Test.Common.Http3LoopbackConnection.HandleRequestAsync(HttpStatusCode statusCode, IList`1 headers, String content) in /_/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs:line 231
at System.Net.Test.Common.Http3LoopbackServer.HandleRequestAsync(HttpStatusCode statusCode, IList`1 headers, String content) in /_/src/libraries/Common/tests/System/Net/Http/Http3LoopbackServer.cs:line 93
at System.Net.Test.Common.Http3LoopbackServer.HandleRequestAsync(HttpStatusCode statusCode, IList`1 headers, String content) in /_/src/libraries/Common/tests/System/Net/Http/Http3LoopbackServer.cs:line 93
at System.Net.Http.Functional.Tests.HttpClientHandler_MaxResponseHeadersLength_Test.<>c__DisplayClass4_0.<<LargeSingleHeader_ThrowsException>b__1>d.MoveNext() in /_/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.MaxResponseHeadersLength.cs:line 88

That's why everything times out, server won't reply until it read the settings frame from the control stream.

The process killing Assert comes from OpenOutboundStream and is for Bidirectional stream, so it's not that.

@MihaZupan MihaZupan force-pushed the fix-maxheaderlength-test branch from 2a2f673 to 4426774 Compare August 15, 2022 19:51
@MihaZupan MihaZupan dismissed ManickaP’s stale review August 15, 2022 19:54

No-merge until the tests are actually fixed

@MihaZupan MihaZupan removed this from the 7.0.0 milestone Aug 15, 2022
@MihaZupan MihaZupan closed this Aug 17, 2022
@MihaZupan MihaZupan reopened this Aug 17, 2022
@MihaZupan MihaZupan closed this Aug 17, 2022
@MihaZupan MihaZupan reopened this Aug 17, 2022
@MihaZupan
Copy link
Member Author

MihaZupan commented Aug 18, 2022

System.Net.Quic.QuicException: An internal error has occurred. Status code: QUIC_STATUS_SUCCESS

😆

console logs

@CarnaViire
Copy link
Member

Note exit code 134 😭
Linking #72696 just in case
Was it happening on other runs as well? @MihaZupan

@ManickaP
Copy link
Member

The SUCCESS status comes from this line: (shutdownByApp: false, closedRemotely: false) => ThrowHelper.GetExceptionForMsQuicStatus(data.ConnectionCloseStatus), Either there's a data corruption or we have bad/old msquic version, or there's a bug in msquic.
cc @rzikm since you added this recently.

@MihaZupan
Copy link
Member Author

This is the first time I've seen that specific failure.
In other runs, it's been System.Net.Quic.QuicException: The connection timed out from inactivity.

@rzikm
Copy link
Member

rzikm commented Aug 18, 2022

cc @rzikm since you added this recently.

I am trying to setup alpine wsl so I can take a look at the dump, but the source suggests that we receive the success status from MsQuic

@MihaZupan MihaZupan closed this Aug 22, 2022
@MihaZupan MihaZupan reopened this Aug 22, 2022
@rzikm
Copy link
Member

rzikm commented Aug 23, 2022

Assert should be fixed by #74348

@MihaZupan MihaZupan added the NO-REVIEW Experimental/testing PR, do NOT review it label Aug 23, 2022
@MihaZupan MihaZupan closed this Aug 23, 2022
@MihaZupan MihaZupan reopened this Aug 23, 2022
@MihaZupan
Copy link
Member Author

Not getting much use out of running CI here, closing for now.

@MihaZupan MihaZupan closed this Aug 23, 2022
@karelz karelz added this to the 8.0.0 milestone Aug 26, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failures in SocketsHttpHandler_HttpClientHandler_MaxResponseHeadersLength_Http3
5 participants