[QUIC] Handle connection abort in streams #52776

CarnaViire · 2021-05-14T17:34:34Z

This leverages newly added flag to SHUTDOWN_COMPLETED event, which means that's not a user-initiated stream shutdown, but a connection abort microsoft/msquic#1581

Fixes #32050

cc @JamesNK: after this PR msquic version should be updated

ghost · 2021-05-14T17:34:42Z

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

Issue Details

This leverages newly added flag to SHUTDOWN_COMPLETED event, which means that's not a user-initiated stream shutdown, but a connection abort microsoft/msquic#1581

Fixes #32050

cc @JamesNK: after this PR msquic version should be updated

Author:	CarnaViire
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

ManickaP

I'm confused, I thought that the whole goal of adding the flag to the stream SHUTDOWN_COMPLETE event was to avoid extra link between stream and a connection. Yet, the MsQuicConnection.State reference is added to MsQuicStream. And if I understand the change right, it's only used to retrieve the connection close error code. Couldn't msquic passed code in the SHUTDOWN_COMPLETE event? And do we really need the code?

CarnaViire · 2021-05-14T19:22:03Z

Couldn't msquic passed code in the SHUTDOWN_COMPLETE event?

@nibanks how hard it will be to add connection's error code to stream's shutdown event?

And do we really need the code?

It is a part of QuicConnectionAbortedException. Unless we want to use another exception type for some reason - I think peer's error code should be delivered to user in this situation also.

geoffkizer · 2021-05-15T00:17:42Z

avoid extra link between stream and a connection.

Why do we want to avoid this? It's not obvious to me why that's bad...

ManickaP · 2021-05-17T08:39:47Z

Why do we want to avoid this?

We asked msquic to change the API (adding the flag to SHUTDOWN_COMPLETE) so that we don't have to hold the link between stream and connection since they have it and can leverage it (or at least that's what I thought was the motivation). With the way the change is done here, we don't actually need the flag, we could just use the connection as we do now for the errorCode:

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

Line 1068 in 104a1be

long errorCode = state.ConnectionState.AbortErrorCode;

So that's why I'm asking. Why did we requested change from msquic? What did we wanted to achieve with it? Because I thought that the goal was to get rid of the link and rely on msquic notification data instead.

It's not obvious to me why that's bad...

I never said it's bad.

CarnaViire · 2021-05-17T18:47:17Z

We asked msquic to change the API (adding the flag to SHUTDOWN_COMPLETE) so that we don't have to hold the link between stream and connection

I originally asked for that so we wouldn't have two-way link (so connection wouldn't have list of streams inside to notify, which was in my first PR).

While it will be great to remove stream->connection link too, #52800 brings it again for another reason...

nibanks · 2021-05-17T18:49:52Z

I don't think you're going to be able to completely remove the interdependency. As you say, #52800 tracks the requirement that you must keep the connection alive at least as long as the streams. I don't want to keep having one-off changes in MsQuic to facilitate "it'd be a little easier if MsQuic did this so I didn't have to go to the connection for this..."

geoffkizer · 2021-05-17T18:51:12Z

I originally asked for that so we wouldn't have two-way link (so connection wouldn't have list of streams inside to notify, which was in my first PR).

I definitely agree that having to maintain a list of streams on the connection would be painful and messy and I hope we never have to do it. So far, it seems we haven't needed to, which is good.

Having a link from the stream to the connection that it's part of seems reasonable, though.

ManickaP · 2021-05-17T19:39:18Z

Am I missing something when I think that the microsoft/msquic#1581 change was not necessary to achieve the solution in this PR?

Edit: I'm not saying that it should be reverted or anything. I'm just trying to understand it all.

nibanks · 2021-05-17T19:43:43Z

Am I missing something when I think that the microsoft/msquic#1581 change was not necessary to achieve the solution in this PR?

Edit: I'm not saying that it should be reverted or anything. I'm just trying to understand it all.

No it was not strictly necessary. The state could have been inferred by the fact that the connection had been shutdown already.

ManickaP

LGTM, pending the decision about connection-stream link.
Comments are mostly code-styling nits, nothing serious.

Thanks!

ManickaP · 2021-05-25T15:28:48Z

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

+
+            if (NetEventSource.Log.IsEnabled())
+            {
+                NetEventSource.Info(_state, $"[Connection#{_state.GetHashCode()}] inbound connection created");


Ohh, this is a prime example where could actually log the handle to be able to easily map it to the msquic logs, see #53224:

on the .NET side I'm wondering if we should show (or log) the handle so it easier to correlate to the native logs.

But that's absolutely not the goal of this PR and you shouldn't worry about it here, just good to be aware of it.

Yes, that's indeed is a good example. But let's update it separately, I'd love this PR to be merged sooner to stop blocking others

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

wfurt · 2021-05-25T17:32:45Z

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

@@ -383,6 +412,14 @@ internal override async ValueTask ShutdownWriteCompleted(CancellationToken cance
        {
            ThrowIfDisposed();

+            lock (_state)


I'm. not sure if this needs to be under lock since we are not modifying the state.
For the check there is IMHO race condition anyway e.g. the state can be changed immediately after the lock before other code executes.

You technically still need the lock to make visible changes from other threads. We could make access to that one field use Volatile.Read/Volatile.Write as an alternative to avoid the lock.

yes. but even with that there is still race to this really becomes best effort IMHO.
I'm Im not sure if this makes any measurable impact so I feel it is ok to keep it for now.

wfurt

LGTM.

Update msquic and use connection shutdown notification

104a1be

CarnaViire requested review from scalablecory, ManickaP, geoffkizer and wfurt May 14, 2021 17:34

dotnet-issue-labeler bot added the area-System.Net label May 14, 2021

ManickaP reviewed May 14, 2021

View reviewed changes

wfurt mentioned this pull request May 15, 2021

keep MsQuicConnection alive when streams are pending #52800

Merged

runfoapp bot mentioned this pull request May 20, 2021

InvokeCodeThatShouldFirEvents_EnsureEventsFired fails on OSX #52710

Closed

ManickaP reviewed May 25, 2021

View reviewed changes

wfurt reviewed May 25, 2021

View reviewed changes

wfurt approved these changes May 25, 2021

View reviewed changes

CarnaViire added 2 commits May 26, 2021 12:04

Merge branch 'main' into quic-connection-abort-v2

1732e5e

Addressing PR feedback

0a8bf46

CarnaViire merged commit 5fc93f4 into dotnet:main May 26, 2021

CarnaViire deleted the quic-connection-abort-v2 branch May 26, 2021 16:08

ghost locked as resolved and limited conversation to collaborators Jun 25, 2021

karelz added this to the 6.0.0 milestone Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QUIC] Handle connection abort in streams #52776

[QUIC] Handle connection abort in streams #52776

CarnaViire commented May 14, 2021

ghost commented May 14, 2021

ManickaP left a comment

CarnaViire commented May 14, 2021

geoffkizer commented May 15, 2021

ManickaP commented May 17, 2021 •

edited

Loading

CarnaViire commented May 17, 2021

nibanks commented May 17, 2021

geoffkizer commented May 17, 2021

ManickaP commented May 17, 2021 •

edited

Loading

nibanks commented May 17, 2021

ManickaP left a comment

ManickaP May 25, 2021

CarnaViire May 26, 2021

wfurt May 25, 2021

scalablecory May 25, 2021

wfurt May 25, 2021

wfurt left a comment

[QUIC] Handle connection abort in streams #52776

[QUIC] Handle connection abort in streams #52776

Conversation

CarnaViire commented May 14, 2021

ghost commented May 14, 2021

ManickaP left a comment

Choose a reason for hiding this comment

CarnaViire commented May 14, 2021

geoffkizer commented May 15, 2021

ManickaP commented May 17, 2021 • edited Loading

CarnaViire commented May 17, 2021

nibanks commented May 17, 2021

geoffkizer commented May 17, 2021

ManickaP commented May 17, 2021 • edited Loading

nibanks commented May 17, 2021

ManickaP left a comment

Choose a reason for hiding this comment

ManickaP May 25, 2021

Choose a reason for hiding this comment

CarnaViire May 26, 2021

Choose a reason for hiding this comment

wfurt May 25, 2021

Choose a reason for hiding this comment

scalablecory May 25, 2021

Choose a reason for hiding this comment

wfurt May 25, 2021

Choose a reason for hiding this comment

wfurt left a comment

Choose a reason for hiding this comment

ManickaP commented May 17, 2021 •

edited

Loading

ManickaP commented May 17, 2021 •

edited

Loading