QUIC read pipeline changes #55505

CarnaViire · 2021-07-12T12:41:38Z

This brings changes to read states and behavior done initially in #52929 with my fixes to it to make all tests work

ghost · 2021-07-12T12:41:42Z

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

Issue Details

This brings changes to read states and behavior done initially in #52929 with my fixes to it to make all tests work

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

CarnaViire · 2021-07-12T17:58:08Z

I've run stress tests on this. It fixes "The response ended prematurely" exceptions (and does not bring any new ones 😁).
More than that, I found out that GET Aborted scenario actually lacked proper checking for HTTP/3 exceptions, so if we treat

HttpRequestException->IOException->HttpRequestException->QuicStreamAbortedException("Stream aborted by peer (258).")
HttpRequestException->QuicStreamAbortedException("Stream aborted by peer (258).")

as expected exceptions (258 = H3_INTERNAL_ERROR (0x102)), then all GET Aborted are passing so far 🥳

client_1  | HttpStress Run Final Report
client_1  |
client_1  | [07/12/2021 17:48:50] Total: 16,218 Runtime: 00:11:00
client_1  |      0: GET                       Success: 1,071    Canceled: 81    Fail: 0
client_1  |      1: GET Partial               Success: 1,082    Canceled: 69    Fail: 0
client_1  |      2: GET Headers               Success: 1,054    Canceled: 97    Fail: 0
client_1  |      3: GET Parameters            Success: 1,067    Canceled: 85    Fail: 0
client_1  |      4: GET Aborted               Success: 1,075    Canceled: 77    Fail: 0
client_1  |      5: POST                      Success: 1,074    Canceled: 79    Fail: 0
client_1  |      6: POST Multipart Data       Success: 1,079    Canceled: 75    Fail: 0
client_1  |      7: POST Duplex               Success: 1,076    Canceled: 79    Fail: 0
client_1  |      8: POST Duplex Slow          Success: 1,069    Canceled: 85    Fail: 0
client_1  |      9: POST Duplex Dispose       Success: 1,080    Canceled: 75    Fail: 0
client_1  |     10: POST ExpectContinue       Success: 0        Canceled: 77    Fail: 0
client_1  |     11: HEAD                      Success: 1,082    Canceled: 73    Fail: 0
client_1  |     12: PUT                       Success: 1,082    Canceled: 71    Fail: 0
client_1  |     13: PUT Slow                  Success: 1,062    Canceled: 90    Fail: 0
client_1  |     14: GET Slow                  Success: 1,077    Canceled: 75    Fail: 0
client_1  |         TOTAL                     Success: 15,030   Canceled: 1,188 Fail: 0

wfurt

it generally looks ok to me. But the state transitions are not easy to verify.
It would be great if also @stephentoub can take a look at the cancellation and pinning.

wfurt · 2021-07-12T22:44:00Z

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

-            // Resettable completions to be used for multiple calls to receive.
-            public readonly ResettableCompletionSource<uint> ReceiveResettableCompletionSource = new ResettableCompletionSource<uint>();
+            // filled when ReadState.BuffersAvailable:
+            public QuicBuffer[] ReceiveQuicBuffers = Array.Empty<QuicBuffer>();


Why did you change List to array? It seems like that would be more difficult to manage in general.

It was Cory's decision. Before, we were clearing List on every read

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

Line 436 in 102fc35

_state.ReceiveQuicBuffers.Clear();

and adding on every RECV event

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

Line 782 in 102fc35

state.ReceiveQuicBuffers.Add(receiveEvent.Buffers[i]);

I guess array way is more performant, so I decided to take in this change.
I may return the list back if you wish.

The array seems fine to me, I'd keep it.

wfurt · 2021-07-12T22:47:06Z

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

+                new Span<byte>(nativeBuffer.Buffer, takeLength).CopyTo(destinationBuffer);
+                destinationBuffer = destinationBuffer.Slice(takeLength);
+            }
+            while (destinationBuffer.Length != 0 && ++i < sourceBuffers.Length);


How do we handle partial data? e.g. We have more data than destinationBuffer buffer. So we consume some but how do we know where to start next time?

Below we return amount we've consumed, and then we call ReceiveComplete(taken); to tell msquic that amount. Then we call EnableReceive(); which will instruct msquic to produce new RECV event with remaining data.

We've done the same thing before, it was just moved around.

The only actual addition is, if we've consumed everything right inside RECV event callback, we tell mquic that right away, without additional EnableReceive()

wfurt · 2021-07-12T22:57:32Z

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


-            lock (_state)
+            switch (readState)


With exception of pre-cancellation the readState seems never updated. Are we getting here only in failed cases?

readState is a copy of an initial value of _state.ReadState. So it is not intended to change, with exception of pre-cancellation, when there's nothing left to do except throw, and throwing is done in one place here. _state.ReadState is changed depending on whether there is data available (IndividualReadComplete->None) or not available (None->PendingRead) or upon cancellation (any->Aborted)

Are we getting here only in failed cases?

Yes (Well, almost, as ReadsCompleted, which is EOS, is a success case). In success case where we have data already, above we return new ValueTask<int>(taken);, in success case where we wait for data, return _state.ReceiveResettableCompletionSource.GetValueTask();

wfurt · 2021-07-12T23:02:27Z

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

+                            QuicBuffer[] oldReceiveBuffers = state.ReceiveQuicBuffers;
+                            state.ReceiveQuicBuffers = ArrayPool<QuicBuffer>.Shared.Rent((int)receiveEvent.BufferCount);
+
+                            if (oldReceiveBuffers.Length != 0) // don't return Array.Empty.


Should we assert somehow the old buffers were fully consumed?

There's no need to do that. Unconsumed data arrives again from the point we've stopped. New RECV event wouldn't come until we call EnableReceive(), and we call it only after we've consumed as much as we could and said so to msquic in ReceiveComplete(taken), so new event will have all the remaining data.

Maybe a comment wouldn't hurt?

ManickaP

Some nits, and suggestion about the code organization, but otherwise looks good and if it works then great 👍

ManickaP · 2021-07-13T14:57:08Z

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

+            // set when ReadState.PendingRead:
+            public Memory<byte> ReceiveUserBuffer;
+            public CancellationTokenRegistration ReceiveCancellationRegistration;
+            public MsQuicStream? RootedReceiveStream; // roots the stream in the pinned state to prevent GC during an async read I/O.


This should be called just Stream or we should rename the equivalent in MsQuicConnection:

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

Line 49 in 594901a

public MsQuicConnection? Connection;

Also, position-wise, it should be higher, next to the handles, to follow the similarity with connection. It's a NIT, but my "consistency" radar is really unhappy about it 😄

ManickaP · 2021-07-13T14:58:04Z

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

+            public Memory<byte> ReceiveUserBuffer;
+            public CancellationTokenRegistration ReceiveCancellationRegistration;
+            public MsQuicStream? RootedReceiveStream; // roots the stream in the pinned state to prevent GC during an async read I/O.
+            public readonly ResettableCompletionSource<int> ReceiveResettableCompletionSource = new ResettableCompletionSource<int>();


Is there a reason why this property lost its comment?

ManickaP · 2021-07-13T15:02:12Z

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

            if (NetEventSource.Log.IsEnabled())
            {
                NetEventSource.Info(_state, $"[Stream#{_state.GetHashCode()}] reading into Memory of '{destination.Length}' bytes.");
            }

+            ReadState readState;
+            long abortError = -1;


Suggested change

long abortError = -1;

long abortError;

ManickaP · 2021-07-13T15:10:50Z

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

+                readState = _state.ReadState;
+                abortError = _state.ReadErrorCode;
+
+                if (readState != ReadState.PendingRead && cancellationToken.IsCancellationRequested)


Why not in PendingRead?
Do we somehow guard against parallel reads? Does this PR contain something that prevents that?
EDIT: I see it in the switch after.

ManickaP · 2021-07-13T15:21:34Z

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

-                {
-                    state.ReceiveResettableCompletionSource.CompleteException(
-                        ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException("Read was canceled", token)));
+                    return _state.ReceiveResettableCompletionSource.GetValueTask();


The combination of if-else if-else if with return at the end of some of the branches is super confusing to me. For instance, the first if (readState ...) will continue after the whole block, but the two else if-else if both end with return, ending the flow there. Could we reshuffle this? Maybe put the two else-if which return as first and make them just ifs.

What is your opinion on the code organization of this? Do you find it easily readable/comprehensible? Maybe I'm just not familiar enough with this style.

As we just talked offline, feel free to keep this as is, even for a follow up. My "feelings" about readability are not sound enough reason for so much work.

ManickaP · 2021-07-13T15:24:56Z

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

+                    ex =
+                        canceledSynchronously ? new OperationCanceledException(cancellationToken) : // aborted by token being canceled before the async op started.
+                        abortError == -1 ? new QuicOperationAbortedException() : // aborted by user via some other operation.
+                        new QuicStreamAbortedException(abortError); // aborted by peer.
+
+                    break;


NIT:

Suggested change

ex =

canceledSynchronously ? new OperationCanceledException(cancellationToken) : // aborted by token being canceled before the async op started.

abortError == -1 ? new QuicOperationAbortedException() : // aborted by user via some other operation.

new QuicStreamAbortedException(abortError); // aborted by peer.

break;

ex = canceledSynchronously ? new OperationCanceledException(cancellationToken) : // aborted by token being canceled before the async op started.

ThrowHelper.GetStreamAbortedException(abortError); // aborted by peer.

break;

ManickaP · 2021-07-13T15:36:29Z

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

            {
-                state.ReceiveQuicBuffers.Add(receiveEvent.Buffers[i]);
+                // This is a 0-length receive that happens once reads are finished (via abort or otherwise).
+                // State changes for this are handled elsewhere.


In PEER_SEND_SHUTDOWN / PEER_SEND_ABORT / SHUTDOWN_COMPLETE event handlers

Can we put it in that comment instead of "elsewhere"? 😄

ManickaP · 2021-07-13T15:45:22Z

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

@@ -1306,6 +1411,16 @@ private static uint HandleEventConnectionClose(State state)
        private static Exception GetConnectionAbortedException(State state) =>
            ThrowHelper.GetConnectionAbortedException(state.ConnectionState.AbortErrorCode);

+        // Read state transitions:


Awesome comment! 🥳

CarnaViire · 2021-07-13T16:45:38Z

I will create a follow-up PR with cosmetic changes

Follow-up for NITs and cosmetic changes from #55505

CarnaViire added 6 commits July 11, 2021 22:51

Update read process and add tests

da979d4

Fix quic tests

0a871bc

Merge fix

9ffc102

Merge branch 'main' into quic-read-fixes

f8659ec

Add read state transitions info

b01d6a4

Fix trailing whitespace

93ba9d9

CarnaViire added the area-System.Net.Quic label Jul 12, 2021

CarnaViire requested review from ManickaP, geoffkizer and wfurt July 12, 2021 12:41

wfurt reviewed Jul 12, 2021

View reviewed changes

runfoapp bot mentioned this pull request Jul 13, 2021

system.linq.expressions.tests work item #55546

Closed

ManickaP approved these changes Jul 13, 2021

View reviewed changes

CarnaViire merged commit 430d87f into dotnet:main Jul 13, 2021

CarnaViire mentioned this pull request Jul 13, 2021

[QUIC] Cosmetic changes to Read pipeline #55591

Merged

ManickaP mentioned this pull request Jul 14, 2021

[QUIC] Intermittent hangs in CI #55642

Closed

CarnaViire added a commit that referenced this pull request Jul 14, 2021

[QUIC] Cosmetic changes to Read pipeline (#55591)

7cfa945

Follow-up for NITs and cosmetic changes from #55505

karelz added this to the 6.0.0 milestone Jul 15, 2021

This was referenced Jul 15, 2021

[QUIC] Optimize stream reads #42651

Closed

HTTP/3: InvalidOperationException with many streams on a connection #55249

Closed

wfurt mentioned this pull request Jul 18, 2021

enable two quic tests #55880

Merged

ManickaP mentioned this pull request Jul 20, 2021

[QUIC] Remove AppContext switch from S.N.Quic #56027

Merged

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QUIC read pipeline changes #55505

QUIC read pipeline changes #55505

CarnaViire commented Jul 12, 2021

ghost commented Jul 12, 2021

CarnaViire commented Jul 12, 2021

wfurt left a comment

wfurt Jul 12, 2021

CarnaViire Jul 13, 2021

ManickaP Jul 13, 2021

wfurt Jul 12, 2021

CarnaViire Jul 13, 2021

wfurt Jul 12, 2021

CarnaViire Jul 13, 2021

wfurt Jul 12, 2021

CarnaViire Jul 13, 2021

ManickaP Jul 13, 2021

ManickaP left a comment

ManickaP Jul 13, 2021

ManickaP Jul 13, 2021

ManickaP Jul 13, 2021

ManickaP Jul 13, 2021

ManickaP Jul 13, 2021

ManickaP Jul 13, 2021

ManickaP Jul 13, 2021

ManickaP Jul 13, 2021

CarnaViire Jul 13, 2021

ManickaP Jul 13, 2021

ManickaP Jul 13, 2021

CarnaViire commented Jul 13, 2021

QUIC read pipeline changes #55505

QUIC read pipeline changes #55505

Conversation

CarnaViire commented Jul 12, 2021

ghost commented Jul 12, 2021

CarnaViire commented Jul 12, 2021

wfurt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManickaP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarnaViire commented Jul 13, 2021