`QuicStream.ReadsClosed` doesn't always complete on connection disposal

[bentoi]

Description

Depending on the amount of data buffered on the stream, awaiting on QuicStream.ReadsClosed might not complete with an exception if the connection is closed/disposed.

Reproduction Steps

Here's the test case project: quictest.zip

Fails with a large payload:

vagrant@buster64-dev-f6caca97:~/workspace/quictest$ dotnet run 200000
Waiting for remote stream reads closed
Unhandled exception. System.Net.Quic.QuicException: Operation aborted.
   at System.Net.Quic.ResettableValueTaskSource.TryComplete(Exception exception, Boolean final)
   at System.Net.Quic.QuicStream.HandleEventShutdownComplete(_SHUTDOWN_COMPLETE_e__Struct& data)
   at System.Net.Quic.QuicStream.HandleStreamEvent(QUIC_STREAM_EVENT& streamEvent)
   at System.Net.Quic.QuicStream.NativeCallback(QUIC_HANDLE* connection, Void* context, QUIC_STREAM_EVENT* streamEvent)
--- End of stack trace from previous location ---
   at Program.Main(String[] args) in /home/vagrant/workspace/quictest/Program.cs:line 64
   at Program.Main(String[] args) in /home/vagrant/workspace/quictest/Program.cs:line 69
   at Program.Main(String[] args) in /home/vagrant/workspace/quictest/Program.cs:line 69
   at Program.Main(String[] args) in /home/vagrant/workspace/quictest/Program.cs:line 69
   at Program.<Main>(String[] args)

Hangs with a small payload:

vagrant@buster64-dev-f6caca97:~/workspace/quictest$ dotnet run 10
Waiting for remote stream reads closed
^CAttempting to cancel the build...

Expected behavior

I expect the same behavior regardless of the payload size.

Actual behavior

It hangs with a small payload.

Regression?

No response

Known Workarounds

No response

Configuration

This occurs on Linux/Debian with .NET 7.0 RC1

Other information

No response

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

Issue Details

---

Description

Depending on the amount of data buffered on the stream, awaiting on QuicStream.ReadsClosed might not complete with an exception if the connection is closed/disposed.

Reproduction Steps

Here's the test case project: quictest.zip

Fails with a large payload:

vagrant@buster64-dev-f6caca97:~/workspace/quictest$ dotnet run 200000
Waiting for remote stream reads closed
Unhandled exception. System.Net.Quic.QuicException: Operation aborted.
   at System.Net.Quic.ResettableValueTaskSource.TryComplete(Exception exception, Boolean final)
   at System.Net.Quic.QuicStream.HandleEventShutdownComplete(_SHUTDOWN_COMPLETE_e__Struct& data)
   at System.Net.Quic.QuicStream.HandleStreamEvent(QUIC_STREAM_EVENT& streamEvent)
   at System.Net.Quic.QuicStream.NativeCallback(QUIC_HANDLE* connection, Void* context, QUIC_STREAM_EVENT* streamEvent)
--- End of stack trace from previous location ---
   at Program.Main(String[] args) in /home/vagrant/workspace/quictest/Program.cs:line 64
   at Program.Main(String[] args) in /home/vagrant/workspace/quictest/Program.cs:line 69
   at Program.Main(String[] args) in /home/vagrant/workspace/quictest/Program.cs:line 69
   at Program.Main(String[] args) in /home/vagrant/workspace/quictest/Program.cs:line 69
   at Program.<Main>(String[] args)

Hangs with a small payload:

vagrant@buster64-dev-f6caca97:~/workspace/quictest$ dotnet run 10
Waiting for remote stream reads closed
^CAttempting to cancel the build...

Expected behavior

I expect the same behavior regardless of the payload size.

Actual behavior

It hangs with a small payload.

Regression?

No response

Known Workarounds

No response

Configuration

This occurs on Linux/Debian with .NET 7.0 RC1

Other information

No response

Author:	bentoi
Assignees:	-
Labels:	area-System.Net.Quic
Milestone:	-

[ManickaP]

I did a quick check on this. The reason is that there are pending data in the read buffer, finished with successful close of the read side (in some cases, e.g. big data send, the connection close is faster than sending and buffering the data, thus you get the exception).
Firstly, the biggest problem is the inconsistency. On the other hand, there are cases where timing and/or varying order of received frames prevents us from being 100% consistent and that's outside of our control.
Secondly, we should make sure that we have the correct/expected behavior of the Reads/WritesClosed task. Since the read data have been all received (including FIN flag) but not consumed, should the task unblock anyway? I'm not trying to say this bug report is wrong, just pointing out that it needs to be thought through and not just blindly fix this one case.
Lastly, I did experiment with putting a call to DisposeAsync before awaiting the task, which should help, but it didn't and that should certainly be fixed:

...
await Console.Out.WriteLineAsync("Waiting for remote stream reads closed");
await remoteStream.DisposeAsync();
await remoteStream.ReadsClosed;
await Console.Out.WriteLineAsync("Waiting for local stream reads closed");
await localStream.ReadsClosed;
...

[bentoi]

Here's my point of view from a developper perspective using the Quic API.

The Quic spec mentions that opened streams should be considered implicitly reset on the receive of the Close frame or on silent close (https://www.rfc-editor.org/rfc/rfc9000.html#immediate-close). So to me, the question is whether or not the Quic implementation should consider a stream with buffered data + FIN closed or opened.

I did expect that the stream would still be considered open since the application either didn't consume the buffered data, abort reads or disposed the stream. And as a result, I did expect the task to fail with the same exception that occurs when the buffered data doesn't include the FIN flag.

I agree this needs to be thought through and I hope my input will be useful for deciding the outcome of this issue 🙂.

[ManickaP]

Triage: this should be sorted out in 8.0.

[ManickaP]

So after a round of discussions with @CarnaViire and @wfurt, we decided, that in case of a connection closure, we will not discard successfully buffered data if we were able to buffer them all (including FIN flag).
QUIC spec leaves the behavior in this case up to the implementation, see optional transition to RESET if all data are received:

       o
       | Recv STREAM / STREAM_DATA_BLOCKED / RESET_STREAM
       | Create Bidirectional Stream (Sending)
       | Recv MAX_STREAM_DATA / STOP_SENDING (Bidirectional)
       | Create Higher-Numbered Stream
       v
   +-------+
   | Recv  | Recv RESET_STREAM
   |       |-----------------------.
   +-------+                       |
       |                           |
       | Recv STREAM + FIN         |
       v                           |
   +-------+                       |
   | Size  | Recv RESET_STREAM     |
   | Known |---------------------->|
   +-------+                       |
       |                           |
       | Recv All Data             |
       v                           v
   +-------+ Recv RESET_STREAM +-------+
   | Data  |--- (optional) --->| Reset |
   | Recvd |  Recv All Data    | Recvd |
   +-------+<-- (optional) ----+-------+
       |                           |
       | App Read All Data         | App Read Reset
       v                           v
   +-------+                   +-------+
   | Data  |                   | Reset |
   | Read  |                   | Read  |
   +-------+                   +-------+