QuicStream shutdown

[scalablecory]

This implements #756, #32075, #42651, and the QuicStream part of #32142.

It adds:

• QuicStream.CompleteWrites().
• QuicStream.Abort(int errorCode, direction).
• QuicStream.CloseAsync() implicitly calls QuicStream.CompleteWrites() then, if writes haven't been aborted, waits for graceful (both sides have reached FIN/abort on their write stream) shutdown.
• QuicStream.CloseAsync(CancellationToken) to allow cancellation of waiting for that graceful shutdown.
• Prevents GC collection of stream when a ReadAsync() and DisposeAsync/CloseAsync is pending.
• Refactors the read code to be more efficient, and fixes some race conditions.

Behaviors introduced

Locally aborting stream, then calling ReadAsync():

Abort();
ReadAsync(); // throws InvalidOperationException("Stream is no longer readable.")

ReadAsync() pending, then locally aborting stream:

(note: we initially wanted this to throw ObjectDisposedException, but because one direction can be aborted without disposing, we need to differentiate the two states)

ValueTask<int> t = ReadAsync();
Abort(123);
await t; // throws QuicOperationAbortedException

Peer aborting stream:

ReadAsync(); // throws QuicStreamAbortedException(int errorCode)

Concurrent reads:

ValueTask<int> t = ReadAsync();
ReadAsync(); // throws InvalidOperationException("Only one read is supported at a time.")

Our recommended disposal pattern is now very complicated... we initially agreed on a design like this but in actual use it's kinda crazy. A sample of it is here:

runtime/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs

Lines 646 to 676 in d9e42a7

	try
	{
	// All the usual stream usage happens inside a try block.
	// Just a dummy throw here to demonstrate the pattern...
	if (abortive)
	{
	throw new Exception();
	}
	}
	catch
	{
	// Abort here. The CloseAsync that follows will still wait for an ACK of the shutdown.
	clientStream.Abort(ExpectedErrorCode, QuicAbortDirection.Both);
	}
	finally
	{
	// Call CloseAsync() with a cancellation token to allow it to time out when peer doesn't shutdown.
	using var shutdownCts = new CancellationTokenSource(500);
	try
	{
	await clientStream.CloseAsync(shutdownCts.Token);
	}
	catch
	{
	// Abort (possibly again, which will ignore error code and not queue any new I/O).
	// This time, Immediate is used which will cause CloseAsync() to not wait for a shutdown ACK.
	clientStream.Abort(ExpectedErrorCode, QuicAbortDirection.Immediate);
	}
	}

[dotnet-issue-labeler]

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]

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

Issue Details

---

This implements #756, #32075, and #42651.

It adds:

• QuicStream.CompleteWrites().
• QuicStream.Abort(int errorCode, direction).
• QuicStream.CloseAsync() implicitly calls QuicStream.CompleteWrites() then, if writes haven't been aborted, waits for graceful (both sides have reached FIN/abort on their write stream) shutdown.
• QuicStream.CloseAsync(CancellationToken) to allow cancellation of waiting for that graceful shutdown.
• Prevents GC collection of stream when a ReadAsync() is pending.
• Refactors the read code to be more efficient.

Behaviors introduced

Locally aborting stream, then calling ReadAsync():

Abort();
ReadAsync(); // throws InvalidOperationException("Stream is no longer readable.")

ReadAsync() pending, then locally aborting stream:

(note: we initially wanted this to throw ObjectDisposedException, but because one direction can be aborted without disposing, we need to differentiate the two states)

ValueTask<int> t = ReadAsync();
Abort();
await t; // throws QuicOperationAbortedException

Peer aborting stream:

ReadAsync(); // throws QuicStreamAbortedException(int errorCode)

Concurrent reads:

ValueTask<int> t = ReadAsync();
ReadAsync(); // throws InvalidOperationException("Only one read is supported at a time.")

Our recommended disposal pattern is now:

await using QuicStream stream = ...;

try
{
   // ... all the usage of the stream.
   await CloseAsync(CancellationToken); // implicitly completes writes. waits for peer shutdown and ACK our shutdown.
}
catch
{
   Abort();

   // now that sends are aborted, the `DisposeAsync` call from the `using` above will close the stream immediately without waiting for peer to shutdown or ACK.

   throw;
}

Author:	scalablecory
Assignees:	-
Labels:	area-System.Net.Quic
Milestone:	6.0.0

[scalablecory]

CC @ManickaP @CarnaViire @wfurt to avoid too many conflicts!

[CarnaViire]

CompleteWrites will not wait for any completion, right? AFAIK we can't remove waiting for completion here and in the other tests. Both sides should receive SHUTDOWN_COMPLETED on app level before Connection.Close is called, otherwise we may get INVALID_STATE exceptions from msquic, because Connection.Close is happening immediately without waiting for anything.

[scalablecory]

This will work because stream.DisposeAsync() now calls shutdown and waits for peer.

[CarnaViire]

If it was a bidi stream and peer did not send their shutdown, we'll wait here forever?

[scalablecory]

Yes. The "recommended disposal behavior" in top comment is how we get around this right now.

The alternative is to make dispose be abortive, which we previously didn't like because other streams treat dispose as graceful.

[CarnaViire]

Looking at the "recommended disposal behavior" - should we then check if CloseAsync was called, which would mean we are on a recommended path -- and if not, perform an abortive dispose?

[CarnaViire]

Reading through the code again, I see that CloseAsync is just DisposeAsync, so the "recommended disposal behavior" is actually just putting a timeout on that wait via CancellationToken that you can't specify otherwise.

[scalablecory]

The problem is that everyone else using Stream -- any stream, not just QuicStream or NetworkStream -- will assume that Dispose() will gracefully close the stream.

We'd be the odd one that made it abortive. So this is a workaround for now to keep consistent. I expect a long deliberation on this one once it gets into API review.

[CarnaViire]

This combination seems invalid... judging by the code, IMMEDIATE is only allowed in combination with both ABORT_SEND and ABORT_RECEIVE
https://github.com/microsoft/msquic/blob/c643c6275dfc506ec47c2067b676f26ba2368568/src/core/api.c#L909-L916

[JamesNK]

https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-4.1-15

A server can send a complete response prior to the client sending an entire request if the response does not depend on any portion of the request that has not been sent and received. When the server does not need to receive the remainder of the request, it MAY abort reading the request stream, send a complete response, and cleanly close the sending part of the stream. The error code H3_NO_ERROR SHOULD be used when requesting that the client stop sending on the request stream.

Would this situation use the new abort API? I'm guessing the server would abort the read side of an incoming bidirectional stream with H3_NO_ERROR.

[karelz]

Triage: We used most of this PR in merged PRs and some leftovers tracked by bugs and new APIs. Closing.