[QUIC] API QuicStream #71969

ManickaP · 2022-07-11T19:42:26Z

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

    
           // TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged 
        
           internal sealed class State 
        
           { 
        
               public long AbortErrorCode = -1; 
        
           } 
        
           private State _state = new State();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

dotnet-issue-labeler · 2022-07-11T19:42:33Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-07-11T19:42:40Z

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

Issue Details

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

    
           // TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged 
        
           internal sealed class State 
        
           { 
        
               public long AbortErrorCode = -1; 
        
           } 
        
           private State _state = new State();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	`new-api-needs-documentation`, `area-System.Net.Quic`
Milestone:	-

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

CarnaViire · 2022-07-12T12:37:00Z

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

+            // Concurrent call, this one lost the race.
+            if (!_receiveTcs.TryGetValueTask(out ValueTask valueTask, this, cancellationToken))
+            {
+                throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "read"));


Why is this inside do...while ? One iteration can copy, and the following can throw?

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

A comment like "this will loop at most twice" would be nice :)

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

CarnaViire · 2022-07-12T12:49:49Z

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


-        public void Shutdown() => _provider.Shutdown();
+        // TODO: memory leak if not disposed
+        _sendBuffers.Dispose();


How do we make sure send buffers are not disposed in the middle of send operation?

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

Can you add a comment for this please? This is something I am going to forget very soon :D

Added comment 3 lines above, at the valueTask await.

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

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

wfurt · 2022-07-13T13:18:35Z

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

-                _stream.Shutdown();
-                await _stream.ShutdownCompleted().ConfigureAwait(false);
-                Dispose();
+                _stream.CompleteWrites();


We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

await using Http3LoopbackStream stream = await connection.AcceptRequestStreamAsync(); await stream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Authentication.cs

wfurt

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic_generated.cs

rzikm

Looks good modulo existing comments

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

rzikm · 2022-07-13T13:56:53Z

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

-        _abortErrorCode = (long)data.ErrorCode;
-        _state.AbortErrorCode = _abortErrorCode;
-        _acceptQueue.Writer.TryComplete(ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetConnectionAbortedException(_abortErrorCode)));
+        _acceptQueue.Writer.TryComplete(ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetConnectionAbortedException((long)data.ErrorCode)));


Do we store the error code anywhere? it might be useful during dump investigation.

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

rzikm · 2022-07-13T14:03:40Z

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

+            // Concurrent call, this one lost the race.
+            if (!_receiveTcs.TryGetValueTask(out ValueTask valueTask, this, cancellationToken))
+            {
+                throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "read"));


A comment like "this will loop at most twice" would be nice :)

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

rzikm · 2022-07-13T14:09:27Z

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


-        public void Shutdown() => _provider.Shutdown();
+        // TODO: memory leak if not disposed
+        _sendBuffers.Dispose();


Can you add a comment for this please? This is something I am going to forget very soon :D

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

ManickaP · 2022-07-13T15:34:27Z

All comments should be answered, only waiting on CI.

CarnaViire · 2022-07-13T17:24:18Z

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

+            await valueTask.ConfigureAwait(false);
+
+            // This is the last read, finish even despite not copying anything.
+            if (complete)


Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

ManickaP · 2022-07-13T18:01:45Z

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

* Quic stream API surface * Fixed test compilation * Fixed http test compilation * HttpLoopbackConnection Dispose -> DisposeAsync * QuicStream implementation * Fixed some tests * Fixed all QUIC and HTTP tests * Fixed exception type for stream closed by connection close * Feedback * Fixed WebSocket.Client test build * Feedback, test fixes * Fixed build on framework and windows * Fixed winhandler test * Swap variable based on order in defining class * Post merge fixes * Feedback and build * Reverted connection state to pass around abort error code * Fixed exception type.

ManickaP · 2022-07-13T18:27:50Z

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.

…72031) (#72106) * [QUIC] API QuicConnection (#71783) * Listener comment; PreviewFeature attribute * Feedback * QuicConnection new API including compilable implementation * Fixed logging * Fixed S.N.Quic and S.N.Http tests * Options now correspond to the issue * Feedback * Comments, PreviewFeature attribute and RemoteCertificate disposal. * Preview feature attribute is assembly wide * Some typos * Fixed test with certificate * Default values as constants * Event handlers split into methods called via switch expression. * Some more comments * Unified unsafe usage * Fixed some more tests * Cleaned up some exceptions and resource strings. * Feedback * Latest greatest API proposal. * Fixed Http solution * Feedback * [QUIC] API QuicStream (#71969) * Quic stream API surface * Fixed test compilation * Fixed http test compilation * HttpLoopbackConnection Dispose -> DisposeAsync * QuicStream implementation * Fixed some tests * Fixed all QUIC and HTTP tests * Fixed exception type for stream closed by connection close * Feedback * Fixed WebSocket.Client test build * Feedback, test fixes * Fixed build on framework and windows * Fixed winhandler test * Swap variable based on order in defining class * Post merge fixes * Feedback and build * Reverted connection state to pass around abort error code * Fixed exception type. * [QUIC] System.Net.Quic API made public (#72031) * System.Net.Quic removed from ASP transport package and made part of SDK ref * Removed manual references to System.Net.Quic.csproj

dotnet-issue-labeler bot added area-System.Net.Quic new-api-needs-documentation labels Jul 11, 2022

ghost assigned ManickaP Jul 11, 2022

CarnaViire reviewed Jul 12, 2022

View reviewed changes

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs Outdated Show resolved Hide resolved

CarnaViire reviewed Jul 12, 2022

View reviewed changes

ManickaP force-pushed the mapichov/quic-stream branch 3 times, most recently from ac8f5ca to ecd1e28 Compare July 12, 2022 20:43

ManickaP added 15 commits July 13, 2022 09:26

Quic stream API surface

69829b6

Fixed test compilation

bbea600

Fixed http test compilation

33e1690

HttpLoopbackConnection Dispose -> DisposeAsync

892ab09

QuicStream implementation

db616fe

Fixed some tests

a9fa895

Fixed all QUIC and HTTP tests

53ace5d

Fixed exception type for stream closed by connection close

1c7eab7

Feedback

3253c03

Fixed WebSocket.Client test build

e8d64bd

Feedback, test fixes

1bbf8af

Fixed build on framework and windows

f048a91

Fixed winhandler test

d9f8890

Swap variable based on order in defining class

0da5ce2

Post merge fixes

d1337e9

ManickaP force-pushed the mapichov/quic-stream branch from 851251d to d1337e9 Compare July 13, 2022 07:36

wfurt reviewed Jul 13, 2022

View reviewed changes

src/libraries/Common/tests/System/Net/Http/Http2LoopbackServer.cs Show resolved Hide resolved

wfurt reviewed Jul 13, 2022

View reviewed changes

src/libraries/Common/tests/System/Net/Http/Http3LoopbackServer.cs Outdated Show resolved Hide resolved

wfurt reviewed Jul 13, 2022

View reviewed changes

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Authentication.cs Outdated Show resolved Hide resolved

wfurt approved these changes Jul 13, 2022

View reviewed changes

rzikm reviewed Jul 13, 2022

View reviewed changes

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

rzikm approved these changes Jul 13, 2022

View reviewed changes

ManickaP added 2 commits July 13, 2022 17:12

Feedback and build

f10b4c7

Reverted connection state to pass around abort error code

34c2492

ManickaP marked this pull request as ready for review July 13, 2022 15:39

This was referenced Jul 13, 2022

[QUIC] API QuicConnection (#71783), QuicStream (#71969), QUIC public (#72031) #72106

Merged

Implement HttpProtocolException for HTTP/3 #72095

Merged

CarnaViire reviewed Jul 13, 2022

View reviewed changes

Fixed exception type.

2e3ad3e

ManickaP merged commit af43652 into dotnet:main Jul 13, 2022

ManickaP deleted the mapichov/quic-stream branch July 13, 2022 18:02

This was referenced Jul 14, 2022

QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103

Closed

[QUIC] Stream refactorings #59346

Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

CarnaViire mentioned this pull request Jul 26, 2022

[QUIC] Read can be highly inefficient #56891

Closed

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QUIC] API QuicStream #71969

[QUIC] API QuicStream #71969

ManickaP commented Jul 11, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Jul 11, 2022

ghost commented Jul 11, 2022

CarnaViire Jul 12, 2022

ManickaP Jul 12, 2022

rzikm Jul 13, 2022

ManickaP Jul 13, 2022

CarnaViire Jul 13, 2022

CarnaViire Jul 12, 2022

ManickaP Jul 12, 2022

rzikm Jul 13, 2022

ManickaP Jul 13, 2022

wfurt Jul 13, 2022

ManickaP Jul 13, 2022

wfurt left a comment

rzikm left a comment

rzikm Jul 13, 2022

ManickaP Jul 13, 2022

rzikm Jul 13, 2022

rzikm Jul 13, 2022

ManickaP commented Jul 13, 2022

CarnaViire Jul 13, 2022

ManickaP Jul 13, 2022

ManickaP commented Jul 13, 2022

ManickaP commented Jul 13, 2022

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internal sealed class State
	{
	public long AbortErrorCode = -1;
	}
	private State _state = new State();

[QUIC] API QuicStream #71969

[QUIC] API QuicStream #71969

Conversation

ManickaP commented Jul 11, 2022 • edited Loading

dotnet-issue-labeler bot commented Jul 11, 2022

ghost commented Jul 11, 2022

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

wfurt left a comment

Choose a reason for hiding this comment

rzikm 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

ManickaP commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManickaP commented Jul 13, 2022

ManickaP commented Jul 13, 2022

ManickaP commented Jul 11, 2022 •

edited

Loading