[QUIC] Remove connection state from QuicStream #72599

ManickaP · 2022-07-21T12:45:18Z

Update msquic to the latest which contains the details in SHUTDOWN_COMPLETE event so we don't have to keep the link between the object anymore.

Also revised some exception and attempted to fix them, please @rzikm check if I'm inline with your QuicException intentions.

Fixes #72238

ghost · 2022-07-21T12:45:56Z

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

Issue Details

Update msquic to the latest which contains the details in SHUTDOWN_COMPLETE event so we don't have to keep the link between the object anymore.

Also revised some exception and attempted to fix them, please @rzikm check if I'm inline with your QuicException intentions.

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

rzikm · 2022-07-21T14:41:04Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs

+            Exception exception = data.ConnectionShutdownByApp != 0 && data.ConnectionClosedRemotely == 0 ?
+                ThrowHelper.GetOperationAbortedException() :
+                // TODO: this will contain 0 for transport shutdown, we should propagate transport error code.
+                ThrowHelper.GetConnectionAbortedException(data.ConnectionShutdownByApp != 0 ? (long)data.ConnectionErrorCode : 0);


We should not throw with ConnectionAborted at all if the connection was shut down by transport. In case of shutdown by transport, we should get the exception with the QuicError corresponding to the CloseStatus provided in the SHUTDOWN_INITIATED_BY_TRANSPORT event on the connection

We didn't have that ability before and we don't have it now.
We do have a transport error code, but we ignore it in both cases (SHUTDOWN_INITIATED_BY_TRANSPORT and here) because we haven't come up with solution where/how to put it.

Yeah, it is something we need to follow-up on, can we put there InternalError for now? I really dislike the idea of Shutdown-by-transport being surfaced as Shutdown-by-application.

I took the liberty of assuming IdleConnection for local transport closure, which from to quick check of msquic looks like it's the most probably cause when you're at the point you already have some streams open (I might have overlooked something and it might obviously change). And I put a comment and linked the issue there. We can solve it for .NET 8.
If you think my assumption is wrong, I'll put InternalError there as well.

If we don't, mind misrepresenting actual protocol violations as idle timeout for now, then I am fine with it.

Once I am done with the task I am currently working on, I would like to take a look at the follow-up, I feel it would be better to try to squeeze it in 7.0.

rzikm · 2022-07-21T14:42:33Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ThrowHelper.cs

-            -1 => GetOperationAbortedException(), // Shutdown initiated by us.
-            long err => new QuicException(QuicError.ConnectionAborted, err, SR.Format(SR.net_quic_connectionaborted, err)) // Shutdown initiated by peer.
-        };
+        return new QuicException(QuicError.ConnectionAborted, errorCode, SR.Format(SR.net_quic_connectionaborted, errorCode));


I would prefer an assert here so that we don't accidentally call this with the old "unset" value of -1, same for the other method

There's no -1 anymore, the code comes straight from msquic.

ManickaP · 2022-07-22T07:46:48Z

/azp list

azure-pipelines · 2022-07-22T07:46:53Z

CI/CD Pipelines for this repository: 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-diffs runtime-coreclr superpmi-asmdiffs-checked-release runtime-manual runtime-extra-platforms jit-cfg perf-wasm runtime-llvm

ManickaP · 2022-07-22T07:47:32Z

/azp run runtime-libraries-coreclr outerloop

ManickaP · 2022-07-22T08:27:12Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-07-22T08:27:25Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP · 2022-07-22T09:18:49Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-07-22T09:18:58Z

Azure Pipelines successfully started running 1 pipeline(s).

rzikm

LGTM, but I would like @CarnaViire to have a look as well before merging

wfurt · 2022-07-24T00:11:35Z

what would happen if somebody runs with older msquic?

ManickaP · 2022-07-24T08:49:21Z

what would happen if somebody runs with older msquic?

It obviously won't work properly, both flags will be 0 (since they were part of RESERVED field and the exception will wrongly be ConnectionIdle.

But I do assume we're gonna release a new version of msquic for .NET 7.0, publish it before GA and check for that version in MsQuicApi.

ManickaP · 2022-07-24T08:50:12Z

Failures are unrelated.

wfurt · 2022-07-24T16:27:09Z

what would happen if somebody runs with older msquic?

It obviously won't work properly, both flags will be 0 (since they were part of RESERVED field and the exception will wrongly be ConnectionIdle.

But I do assume we're gonna release a new version of msquic for .NET 7.0, publish it before GA and check for that version in MsQuicApi.

I think that would be OK as far as we don't cause access violation or some other fatal error. We can be clear about what is minimal supported msquic version.

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs

CarnaViire

LGTM, thanks!

dotnet-issue-labeler bot added the area-System.Net.Quic label Jul 21, 2022

ghost assigned ManickaP Jul 21, 2022

ManickaP requested review from CarnaViire, wfurt and rzikm July 21, 2022 12:45

Removed connection state, updated mqsuic and fixed exceptions

33fa788

ManickaP force-pushed the mapichov/update-msquic branch from a50d816 to 33fa788 Compare July 21, 2022 12:48

rzikm reviewed Jul 21, 2022

View reviewed changes

Feedback

0a4fb12

rzikm approved these changes Jul 22, 2022

View reviewed changes

runfoapp bot mentioned this pull request Jul 22, 2022

Failing test system.tests.gcextendedtests.getallocatedbytesforcurrentthread #65183

Closed

CarnaViire reviewed Jul 25, 2022

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs Outdated Show resolved Hide resolved

CarnaViire reviewed Jul 25, 2022

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs Show resolved Hide resolved

CarnaViire approved these changes Jul 25, 2022

View reviewed changes

ManickaP added 2 commits July 26, 2022 09:12

Merge branch 'main' into mapichov/update-msquic

65f7107

Feedback

d5b534d

ManickaP merged commit 39d82c9 into dotnet:main Jul 26, 2022

ManickaP deleted the mapichov/update-msquic branch July 26, 2022 18:07

rzikm mentioned this pull request Aug 2, 2022

QUIC: Connection timeout has QuicError.InternalError #73141

Closed

karelz added this to the 7.0.0 milestone Aug 7, 2022

ghost locked as resolved and limited conversation to collaborators Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QUIC] Remove connection state from QuicStream #72599

[QUIC] Remove connection state from QuicStream #72599

ManickaP commented Jul 21, 2022 •

edited

Loading

ghost commented Jul 21, 2022

rzikm Jul 21, 2022 •

edited

Loading

ManickaP Jul 22, 2022

rzikm Jul 22, 2022

ManickaP Jul 22, 2022

ManickaP Jul 22, 2022

rzikm Jul 22, 2022

rzikm Jul 21, 2022

ManickaP Jul 22, 2022

ManickaP commented Jul 22, 2022

azure-pipelines bot commented Jul 22, 2022

ManickaP commented Jul 22, 2022

ManickaP commented Jul 22, 2022

azure-pipelines bot commented Jul 22, 2022

ManickaP commented Jul 22, 2022

azure-pipelines bot commented Jul 22, 2022

rzikm left a comment

wfurt commented Jul 24, 2022

ManickaP commented Jul 24, 2022

ManickaP commented Jul 24, 2022

wfurt commented Jul 24, 2022

CarnaViire left a comment

[QUIC] Remove connection state from QuicStream #72599

[QUIC] Remove connection state from QuicStream #72599

Conversation

ManickaP commented Jul 21, 2022 • edited Loading

ghost commented Jul 21, 2022

rzikm Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

ManickaP Jul 22, 2022

Choose a reason for hiding this comment

rzikm Jul 22, 2022

Choose a reason for hiding this comment

ManickaP Jul 22, 2022

Choose a reason for hiding this comment

ManickaP Jul 22, 2022

Choose a reason for hiding this comment

rzikm Jul 22, 2022

Choose a reason for hiding this comment

rzikm Jul 21, 2022

Choose a reason for hiding this comment

ManickaP Jul 22, 2022

Choose a reason for hiding this comment

ManickaP commented Jul 22, 2022

azure-pipelines bot commented Jul 22, 2022

ManickaP commented Jul 22, 2022

ManickaP commented Jul 22, 2022

azure-pipelines bot commented Jul 22, 2022

ManickaP commented Jul 22, 2022

azure-pipelines bot commented Jul 22, 2022

rzikm left a comment

Choose a reason for hiding this comment

wfurt commented Jul 24, 2022

ManickaP commented Jul 24, 2022

ManickaP commented Jul 24, 2022

wfurt commented Jul 24, 2022

CarnaViire left a comment

Choose a reason for hiding this comment

ManickaP commented Jul 21, 2022 •

edited

Loading

rzikm Jul 21, 2022 •

edited

Loading