[QUIC] Design of Shutdown for QuicStreams in System.Net.Quic

[jkotalik]

Problem

So today, we have an API for Quic which looks like the following:

    public sealed class QuicStream : System.IO.Stream
    {
        internal QuicStream() { }
        public override bool CanSeek => throw null;
        public override long Length => throw null;
        public override long Seek(long offset, System.IO.SeekOrigin origin) => throw null;
        public override void SetLength(long value) => throw null;
        public override long Position { get => throw null; set => throw null; }
        public override bool CanRead => throw null;
        public override bool CanWrite => throw null;
        public override void Flush() => throw null;
        public override int Read(byte[] buffer, int offset, int count) => throw null;
        public override void Write(byte[] buffer, int offset, int count) => throw null;
        public long StreamId => throw null;
        public void AbortRead() => throw null;
        public ValueTask ShutdownWriteAsync(System.Threading.CancellationToken cancellationToken = default) => throw null;
    }

Today, QuicStream derives from Stream, which is really nice for interop purposes with other things, allowing people to just read and write to and from the stream without thinking what the underlying implementation is.

When calling Dispose on the stream, that currently triggers an abortive shutdown sequence on the stream rather than graceful. To trigger a graceful shutdown, one needs to call ShutdownWriteAsync, which sends a FIN to the peer and waits for an ACK. However, people that would normally be using a stream wouldn't have access to ShutdownWriteAsync, hence all actions would be abortive by default.

Current thoughts

Ideally for the workflow we'd like to enable is something like:

using (Stream stream = connection.AcceptAsync()) // this is actually a quic stream
{
    Memory<byte> memory = new Memory<byte>(new byte[10]);
    int res = await stream.ReadAsync(memory);
    await stream.WriteAsync(memory);
}

The big question here is how do we make it so we can send a shutdown for a final write with a stream API outside of Dispose?

The first pivot would be whether we'd like QuicStream to derive from stream at all. If we'd like it to derive from System.IO.Stream, I think it would have to make Dispose/DisposeAsync do a graceful shutdown by default in that case, and expose Abortive versions of these functions on the QuicStream itself. That way, in the default case, using the stream APIs "just works". However, doing a graceful shutdown of a stream in Dispose has downsides. We would be doing a network call in Dispose, which would block. DisposeAsync also can't be canceled as well.

So let's talk about a world where QuicStream didn't derive from System.IO.Stream and we had a few liberties with the API. Potentially, we could change a few things:

• Make WriteAsync take a flag which says if this is the final write.
• Allow Dispose to be abortive.

Abortive Dispose wouldn't block at all, which is nice. However, we do need to be careful about how to supply an error code when aborting a stream. The error code that is used for abort is supposed to be "application specific and provided", meaning we'd need a default error code.

cc @dotnet/http3 and @nibanks I haven't really reached a conclusion based on this yet. I'd be interested in your thoughts.

[scalablecory]

Q: an abortive close also needs I/O, doesn't it? Is it managed behind the scenes, or does it also need to be async?

If we implement Stream -- and we probably should, because so many things can use it -- then Dispose and DisposeAsync should be graceful to match the expected behavior of Stream. We just need to live with that extra I/O, I think :/.

Lets look at NetworkStream.Dispose:

• It performs a graceful shutdown, so your sample code works great.
• It doesn't block, because of OS magic, unless you set linger options.
• If you're calling it during an exception-triggered unwind and your protocol doesn't have ways to distinguish a graceful shutdown from a busted protocol, you're going to have a bad time.
  • You need to set ownsSocket=false and manage shutdown yourself to get around this.

Spitballing... if we really want to keep QuicStream deriving from Stream, what we could do is enable an 'advanced' behavior like so:

using QuicStream stream = CreateUnidirectionalStream(abortive = true);

// if exception happens, we don't ever Shutdown() and so Dispose will be abortive.
DoSomethingThatCanThrow();

stream.Shutdown(SD_BOTH); // graceful shutdown, now Dispose is locked in to no-op.

[jkotalik]

an abortive close also needs I/O, doesn't it? Is it managed behind the scenes, or does it also need to be async?

It does IO, but you don't need to await as it's abortive and it's managed behind the scenes.

We just need to live with that extra I/O, I think :/.

So there could be another option which could make it so we don't synchronously block in the Dispose call. Let's say Dispose didn't actually complete the graceful shutdown and kept most of the state, and rather just started it. On the callback to the graceful shutdown completing, it would finish the callback and call stream close.

I really don't like this option because it could lose writes if the connection is closed immediately after calling Dispose. Closing the connection doesn't wait for all streams to gracefully close as well.

Spitballing... if we really want to keep QuicStream deriving from Stream, what we could do is enable an 'advanced' behavior like so:

My feeling is no matter what we'd have to expose another way to do async graceful shutdown on QuicStream. We can track the current state of QuicStream to check whether to noop in Dispose or not.

[nibanks]

IMO, the most important thing that needs to be ironed out is the dispose path here. Lots of focus seems to be on the write path, whether it should be graceful or abortive, but not much on the read path.

A very important point with QUIC is that all aborts (in the read or write direction) must have an application layer error code. Therefore they cannot happen in response to a simple Dispose call, since it takes no error code as argument. If this must is ever violated (say, by closing the stream without explicitly aborting first, or waiting on the graceful close to complete) then the entire connection must be immediately torn down.

Therefore the API must be designed so that it should be very hard to accidentally code up something that either doesn't abort both directions or doesn't gracefully complete them. I don't have a good solution to this that fits inside the Stream interface.

[jkotalik]

A very important point with QUIC is that all aborts (in the read or write direction) must have an application layer error code. Therefore they cannot happen in response to a simple Dispose call, since it takes no error code as argument. If this must is ever violated (say, by closing the stream without explicitly aborting first, or waiting on the graceful close to complete) then the entire connection must be immediately torn down.

Yeah, even in the case where Dispose is graceful by default, there are still conditions where Dispose may be in an abortive state (timeout, didn't read the entire read side, etc.). We'd probably need to supply a default error code that is application specific in that case: maybe as a options when creating a QuicStream on the client or creating the Listener on the Server.

[geoffkizer]

The first pivot would be whether we'd like QuicStream to derive from stream at all.

We want QuicStream to derive from Stream. There's a ton of code that works in terms of Stream, and user understand how Streams work.

But even if we decided that QuicStream didn't actually derive from Stream, we would just turn around and write a wrapper over QuicStream that implements Stream, which would induce all of these questions anyway. In other words, not deriving from Stream doesn't actually solve anything here.

[geoffkizer]

A very important point with QUIC is that all aborts (in the read or write direction) must have an application layer error code. Therefore they cannot happen in response to a simple Dispose call, since it takes no error code as argument.

I agree that this is an important point to consider.

In particular, I think we will need AbortRead(int errorCode) that causes the read side to abort and send a STOP_SENDING frame, and AbortWrite(int errorCode) that causes the write side to abort and send a RESET_STREAM frame.

That said, we're going to need to do something in Dispose if either the read side or write side hasn't completed yet. That means picking some error code and sending this with the STOP_SENDING frame. As @jkotalik suggested above, we can make this configurable if that makes sense, but even then there will need to be some default, likely 0. I think that's fine; if users need more than that, they should call AbortRead or AbortWrite themselves.

[nibanks]

but even then there will need to be some default, likely 0. I think that's fine

I cannot stress enough how absolutely not fine that is. The QUIC spec uses no uncertain terms that the application protocol must be the one to specify this value.

As a transport QUIC has no knowledge of this error code space. 0 might mean "everything was successful! Please continue purchasing that Ferrari." It might mean "the world has ended, please delete the user's account." Since this error code will be delivered to the peer, and the peer will see it explicitly as "your peer aborted the stream and chose to use error code X" the app layer must be the one to choose.

So I see only one of two options if Dispose is somehow called while the stream is still in use:

1. Just call StreamClose and MsQuic will terminate the entire connection.
2. Have a default value that the app must set before it got this far. Perhaps it's part of a constructor somewhere.

[geoffkizer]

@nibanks Is there some reason QUIC hasn't just defined 0 as a universal error code meaning "unknown" or something like that? This seems useful.

[nibanks]

The entire error space is explicitly application owned. QUIC has no insights to the values. There are no default values. Feel free to open an issue on the QUIC spec. I opened one related to this, and any changes were shot down: quicwg/base-drafts#3291

[geoffkizer]

Thanks @nibanks. I commented on the linked issue.