[QUIC] Update to msquic 2 by ManickaP · Pull Request #67383 · dotnet/runtime

ManickaP · 2022-03-31T12:30:45Z

Please review, but don't merge it yet, I need to update our testing docker images first.

ghost · 2022-03-31T12:30:53Z

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

Issue Details

Please review, but don't merge it yet, I need to update our testing docker images first.

Author:	ManickaP
Assignees:	-
Labels:	`NO-MERGE`, `area-System.Net.Quic`
Milestone:	-

ManickaP · 2022-03-31T12:32:46Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

We still get SHUTDOWN_COMPLETE if the connection start fails. If we free the context object here, the callback gets random memory and blows up.

Can we please leave this as a comment in the code?

I don't think it makes sense now since we only free the GC handle in failed ctors and in SHUTDOWN_COMPLETE, which makes sense.
Before this change, the call to free in failed connect was rather out-of-place.

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs

ManickaP · 2022-03-31T12:35:39Z

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs

        }

        [Fact]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/67302")]


We need to wait for stream start callback event in all cases.

ManickaP · 2022-03-31T12:41:57Z

@wfurt looking at this, we should already be consuming latest and greatest msquic:
https://github.com/dotnet/dotnet-buildtools-prereqs-docker/blob/dcc36f08baa367d98f9c6cfcdde28fc0d5f6c6d9/src/fedora/34/helix/amd64/Dockerfile#L25-L34

How come it didn't broke the tests? Or is it because we need to update the image tag in our test pipeline and we haven't done it yet?

EDIT: I'm looking at our registry here https://mcr.microsoft.com/v2/dotnet-buildtools/prereqs/tags/list and the newest fedora-34 image (fedora-34-helix-20211214164220-4f64125) seems to contain only msquic 1.9, so I guess we need to somehow force build of this image.

EDIT2: filed an issue: dotnet/dotnet-buildtools-prereqs-docker#593

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs

rzikm

LGTM, just few questions

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs

rzikm · 2022-03-31T12:51:36Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs


                Debug.Assert(!Monitor.IsEntered(_state), "!Monitor.IsEntered(_state)");
-                status = MsQuicApi.Api.StreamStartDelegate(_state.Handle, QUIC_STREAM_START_FLAGS.FAIL_BLOCKED);
+                status = MsQuicApi.Api.StreamStartDelegate(_state.Handle, QUIC_STREAM_START_FLAGS.FAIL_BLOCKED | QUIC_STREAM_START_FLAGS.SHUTDOWN_ON_FAIL);


Since the stream start is asynchronous now, do we need to do anything to prevent stream operations before we get the StartComplete callback?

Most possibly, but I believe it will be part of #67302 (I also mentioned it in additional thoughts there)

msquic can handle stream operation before the stream is started, so we don't have to prevent anything. But tests and behavior depending on max stream limit is broken now. #67302 should be of very high priority now thanks to this.

behavior depending on max stream limit is broken now

only throwing on reaching the limit is broken, right? the wait loop implemented in H3 should be fine? @ManickaP

That's my understanding.

FYI, it might be worth having a conversation about the FAIL_BLOCKED usage here. MsQuic allows you to start a stream and send on it before starting a connection (practically a requirement for 0-RTT scenarios), and without this flag it allows you to queue safely. But with this flag, it would just fail because you are blocked on starting the connection. It might make sense to use this in your HTTP/3 scenarios, but not universally.

Yes, we do plan to rework stream starting in #67302. We have more issues around this than just 0-RTT 😄

CarnaViire · 2022-03-31T13:02:59Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

            public MsQuicConnection.State ConnectionState = null!; // set in ctor.
            public string TraceId = null!; // set in ctor.

+            public uint StartStatus = MsQuicStatusCodes.Success;


Is it intentional that it will be "success" even before the event arrives?

Yes, we check it only in shutdown complete to unblock pending operations with proper exception. There's no point in distinguishing between start not called yet and called and succeeded.
I also assume this will get reworked with #67302.

CarnaViire

LGTM, thanks!

ManickaP · 2022-04-01T08:14:05Z

/azp list

azure-pipelines · 2022-04-01T08:14:10Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr gcstress0x3-gcstress0xc runtime-coreclr gcstress-extra runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging runtime-coreclr pgo runtime-coreclr libraries-pgo coreclr-gc-regions Antigen runtime-community Fuzzlyn runtime-coreclr superpmi-replay runtime-wasm runtime-coreclr superpmi-asmdiffs runtime-coreclr superpmi-asmdiffs-checked-release runtime-manual runtime-extra-platforms jit-cfg perf-wasm

ManickaP · 2022-04-01T08:14:59Z

/azp run runtime-libraries stress-http

azure-pipelines · 2022-04-01T08:15:08Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP · 2022-04-01T08:24:38Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-04-01T08:24:51Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP · 2022-04-01T08:25:10Z

/azp run runtime

azure-pipelines · 2022-04-01T08:25:36Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2022-04-01T13:59:00Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicEnums.cs

    internal enum QUIC_STREAM_START_FLAGS : uint
    {
        NONE = 0x0000,
-        FAIL_BLOCKED = 0x0001, // Only opens the stream if flow control allows.


We should look into using MsQuic's generated interop layer instead of doing all this manually in .NET. Several of these enums are out of date, and don't include all the latest values.

Yes, at first we've planned to do both of the changes together, but later decided to split to a separate issue #67377

Sounds good!

nibanks · 2022-04-01T14:02:24Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs

+            // TODO: solve listener stopping in better way now that it receives STOP_COMPLETED event.
+            StopAsync().GetAwaiter().GetResult();


We added this async stop functionality at the request of .NET. 😄 We were told it would make things easier for you!

Yes, I have it in my TODO list and plan to address. This is just a hack to get it working and be able to move forward, I'm aware it's ugly as heck 😃

ManickaP · 2022-04-01T15:15:28Z

Outerloop failure are badssl.com is down: #63224
H/3 stress didn't run due to docker issues me and @rzikm observed locally, we should investigate more: #67442

CarnaViire · 2022-04-01T15:31:11Z

src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs

                // So nothing to do.
                return;
            }
-            catch (OperationCanceledException)


I believe we should bring this back? #58078 is not fixed yet

I'll bring it back, but interestingly I haven't seen it in any of my local runs nor pipelines...

It's a race that is not so easy to catch I guess 😅 but it definitely exists

CarnaViire

LGTM, thanks!

ManickaP · 2022-04-01T18:49:02Z

Failing tests seem like fallout from #67037, I'll revert the change around EnableReceive and we'll see if it helps.
My theory is that at that time of #67037 we already had updated msquic in the transport package for Windows so the CI didn't run the tests there (IsSupported would fail due to version incompatibility) and this is actually the first time they are run.

~~If my theory proves true, @rzikm you might want to revisit this.~~

~~BTW, it seems to work on Linux, in CI and locally.~~

Theory was wrong, change reverted in #67481, ReceiveComplete became void. For some reason, the last runtime pipeline run on this PR didn't fail for me.

ManickaP added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 31, 2022

ghost added the area-System.Net.Quic label Mar 31, 2022

ghost assigned ManickaP Mar 31, 2022

ManickaP commented Mar 31, 2022

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs Outdated Show resolved Hide resolved

Update to msquic 2, test and code fixes

bfdbd3f

ManickaP force-pushed the mapichov/msquic_2 branch from f2144c3 to bfdbd3f Compare March 31, 2022 12:34

ManickaP commented Mar 31, 2022

View reviewed changes

CarnaViire reviewed Mar 31, 2022

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs Outdated Show resolved Hide resolved

rzikm approved these changes Mar 31, 2022

View reviewed changes

CarnaViire reviewed Mar 31, 2022

View reviewed changes

feedback

6d96cf7

CarnaViire approved these changes Mar 31, 2022

View reviewed changes

ManickaP mentioned this pull request Mar 31, 2022

Force rebuild of an image dotnet/dotnet-buildtools-prereqs-docker#593

Closed

ManickaP added 3 commits March 31, 2022 18:02

Fixed more HTTP/3 bugs

165dfa6

updated fedora image to contain the newest msquic

21de236

Better test debugging, fixed some timeouts

616be2f

ManickaP force-pushed the mapichov/msquic_2 branch from 2698330 to 616be2f Compare March 31, 2022 18:55

nibanks reviewed Apr 1, 2022

View reviewed changes

ManickaP removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 1, 2022

CarnaViire reviewed Apr 1, 2022

View reviewed changes

feedback

ccb198a

CarnaViire approved these changes Apr 1, 2022

View reviewed changes

Return some msquic calls into a lock

dbdfdfe

ManickaP merged commit d5b1a7f into dotnet:main Apr 1, 2022

ManickaP deleted the mapichov/msquic_2 branch April 1, 2022 21:26

jkotas mentioned this pull request Apr 2, 2022

Quic tests failing with Error Code: 0xEF850AE0 Stream aborted by peer #67476

Closed

karelz added this to the 7.0.0 milestone Apr 8, 2022

ghost locked as resolved and limited conversation to collaborators May 8, 2022

		// TODO: solve listener stopping in better way now that it receives STOP_COMPLETED event.
		StopAsync().GetAwaiter().GetResult();

Conversation

ManickaP commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaP commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rzikm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarnaViire left a comment

Choose a reason for hiding this comment

Uh oh!

ManickaP commented Apr 1, 2022

Uh oh!

azure-pipelines bot commented Apr 1, 2022

Uh oh!

ManickaP commented Apr 1, 2022

Uh oh!

azure-pipelines bot commented Apr 1, 2022

Uh oh!

ManickaP commented Apr 1, 2022

Uh oh!

azure-pipelines bot commented Apr 1, 2022

Uh oh!

ManickaP commented Apr 1, 2022

Uh oh!

azure-pipelines bot commented Apr 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaP commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarnaViire left a comment

ManickaP commented Mar 31, 2022 •

edited

Loading

ManickaP commented Mar 31, 2022 •

edited

Loading

ManickaP commented Apr 1, 2022 •

edited

Loading

ManickaP commented Apr 1, 2022 •

edited

Loading