HTTP/3: Avoid per-request cancellation token allocations

[JamesNK]

Fixes #42652
Fixes #42725

Add IConnectionCompleteFeature to QUIC stream abstraction and use it to get complete notification instead of ConnectionClosed cancellation token.

[JamesNK]

After:

Note no cancellation token, registration or node allocations.

[halter73]

Are we still upholding this contract by firing this here? It seems like this might get invoked too early.

IConnectionLifetime ConnectionClosed will fire immediately, possibly in the middle of requests. Trying to use it for cleanup would cause lots of race conditions and ObjectDisposedExceptions up stack. OnCompleted acts as a finally block for the connection, allowing you to clean up resources once you're sure everybody is done using them.

#9754 (comment)

@Tratcher

[JamesNK]

The callback is invoked in FireStreamClosedAsync which is called after the send and receive loops are complete. At this point the input/output have either been completed, either successfully or with an error.

    private async Task StartAsync()
    {
        Debug.Assert(_stream != null);

        try
        {
            // Spawn send and receive logic
            // Streams may or may not have reading/writing, so only start tasks accordingly
            var receiveTask = Task.CompletedTask;
            var sendTask = Task.CompletedTask;

            if (_stream.CanRead)
            {
                receiveTask = DoReceive();
            }

            if (_stream.CanWrite)
            {
                sendTask = DoSend();
            }

            // Now wait for both to complete
            await receiveTask;
            await sendTask;

            await FireStreamClosedAsync();
        }
        catch (Exception ex)
        {
            _log.LogError(0, ex, $"Unexpected exception in {nameof(QuicStreamContext)}.{nameof(StartAsync)}.");
        }
    }

The only later place I can think to place it would be in QuicStreamContext.Dispose(). That doesn't seem useful as HTTP/3 layer calls dispose so it isn't useful as a callback.

[halter73]

This is wrong if it means that the callbacks can be called before the ConnectionDelegate completes. IConnectionCompleteFeature.OnCompleted is semantically different from ConnectionContext.ConnectionClosed which should fire as soon as I/O completes.

There's also the concern that any user hooking IConnectionCompleteFeature.OnCompleted via HttpContext.Features (assuming this is exposed through that) might expect the HTTP/1.x and HTTP/2 behavior where it only fires after the overall connection is complete and not just a request/stream.

Even if we just used this internally and hid it from all users including not just middleware but als low-level IMultiplexedConnectionListenerFactory consumers, I still don't like changing the semantics of IConnectionCompleteFeature to let it ever fire OnCompleted callbacks before the ConnectionDelegate completes. Better to just make a new internal interface if we need a non-allocating ConnectionClosed because that's not what IConnectionCompleteFeature is.

[JamesNK]

This is HTTP/3 and multiplexed. Is there an equivalent of ConnectionDelegate in this situation? There is a request delegate, but that's a HTTP/3 thing, not transport.

A new interface can't be internal. These types are in Transport.Quic and we're consuming them in Kestrel.Core. Needs to be added to Microsoft.AspNetCore.Connections.Abstractions. That's why I was hoping to reuse an existing interface.

If we're adding something new, how about:

public interface IStreamClosedFeature
{
    void OnClosed(Func<object, Task> callback, object state);
}

Note that HTTP/3 doesn't need to run any async logic so that the callback could be Action<object> instead. But returning a task leaves open the possibility of doing that in the future. The downside is what happens if there is a non-async situation where we want to raise this event from. Pros and cons.

Thoughts?

[halter73]

Good point about the lack of a ConnectionDelegate equivalent. Neither ConnectionDelegate nor MultiplexedConnectionDelegate mean anything to the transports or the ConnectionContexts they implement. These delegate types are just a convenient way to pass in the logic you want Kestrel to run when using these transports. Nothing in Kestrels transports today raise IConnectionCompleteFeature.OnCompleted callbacks. Instead, that's done in Kestrel.Core by ConnectionDispatcher in a finally block after running the ConnectionDelegate.

I think the most equivalent thing that could be done for HTTP/3 would be to raise IConnectionCompleteFeature.OnCompleted callbacks at the end of Http3Stream.ProcessRequestAsync. I think this would be okay as long as it doesn't leak through HttpContext.Features, but then it stops being useful to you as a ConnectionClosed replacement.

I'm okay with IStreamClosedFeature. Unless you have an even better alternative, I suggest throwing it in an API proposal issue.

[JamesNK]

#42725

[JamesNK]

PR updated to use IStreamClosedFeature. PTAL.

[adityamandaleeka]

I know this isn't new in this code, but why is this checking for >= 0 instead of != -1?

[JamesNK]

Probably because anything negative in an error. QUIC only allows positive error codes.

[adityamandaleeka]

Maybe consider naming this _onClosedHandlers or something?

[JamesNK]

Sure. I'll change it in another PR to avoid rebuilding this one in CI.

[ghost]

@JamesNK, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!