HTTP2: Support bidirectional content

[Tratcher]

Update & re-scoping of the issue

#1511 (comment)
Remaining work: We added an internal AllowDuplex property on HttpContent. We should consider making this public.

Old Update

As part of 3.0 we essentially disabled bidirectional HttpContent, implementing only what was needed for gRPC support. We now need to, as part of 5.0/Future, consider how to enable general-purpose bidirectional support.

Original issue

dotnet/corefx#36884 (HTTP/2 100 Continue) also snuck in bidirectional support (https://github.com/dotnet/corefx/issues/36855) for HTTP/2. This works pretty well but does introduce an ambiguous scenario when the server completes the response before the client completes the request.

Server code: (Kestrel 3.0.0-preview7)

                var response = "hello, world";
                context.Response.ContentLength = response.Length;
                await context.Response.WriteAsync(response);

                await context.Features.Get<IHttpResponseCompletionFeature>().CompleteAsync();
                await Task.Delay(TimeSpan.FromSeconds(5));
                // Drain the request body
                await context.Request.Body.CopyToAsync(Stream.Null);

Yes this server code is a bit unusual, it was written this way to make the error visible from CopyToAsync. You'll get similar behavior if you just write Hello World and let the request unwind.

Client code:

            var client = new HttpClient();
            client.DefaultRequestVersion = new Version(2, 0);
            using var file = File.OpenRead(@"A200MbFile.txt");
            var content = new StreamContent(file);
            var response = await client.PostAsync("https://localhost:5001", content);

• In HTTP/1.x this works fine, PostAsync won't complete until the request stream has been completely uploaded.
• In HTTP/2 it's ambiguous because when PostAsync completes you have a fully buffered response body but you have no idea what the state of the request is nor a way to be notified of its completion. Trying to perform any operations on that file can cause errors in the client code because it's still in use. If you close the file now the request will fail, but the error will only be visible on the server.
• HttpCompletionOptions do not have any affect on the request, they only affect the response.
• None of the built in HttpContent implementations provide any visibility on their progress. PushStreamContent from WebApi is the only HttpContent example I've seen that would surface this state to users.

Mitigations:

• Servers usually wait for the request body to end before ending the response body. However the client can't always rely on the server behaving a specific way, ending the response early is within spec.
• For >= 300 responses the HTTP/2 stream will be reset and the request stream abandon. These are the most common scenarios where the server would end the response without draining the request. This doesn't surface any errors directly to the caller unless they have a custom HttpContent, they have to infer the request body is done/abandon from the status code.

These mitigations make the issue occur infrequently enough that it will cause some very non-obvious issues when it it does happen (e.g. locked files, InvalidOperationExceptions, multi-threaded stream re-use, etc.).

Proposals:
A) At a minimum there needs to be guidance given that this might happen and how the user might handle it. I don't know what guidance could be given beyond implementing a custom HttpContent.
B) Add something to HttpContent to make progress more visible. E.g. something they could await or DisposeAsync that could wait for things to finish first. This wouldn't be very discoverable and would need to be accompanied by guidance.
C) Only enable bidirectional support when using HttpCompletionOptions.ResponseHeadersRead (or a new flag?). Most people don't use this flag so they wouldn't be impacted by the behavior change (e.g. PostAsync doesn't support that mode). I'm not sure how you implement this without B.

@wfurt @davidsh

[wfurt]

The original implementation did C) but consensus through review was to do it al the time.

cc: @stephentoub

[stephentoub]

How about:

D) Don't let the response stream yield EOF until all request content has finished sending.

Would that break anything? It'd also give us a place to propagate exceptions we're currently just logging.

[Tratcher]

D is certainly simpler, and doesn't depend on any of the other proposals. It might break the gRPC scenario where they send an error status in the trailers. However they usually follow that with a Reset so that might unblock it anyways. @JamesNK ?

[JamesNK]

Don't let the response stream yield EOF

What do you mean by this?

I'll put together some gRPC streaming scenarios so we can judge if changes will break anything.

[JamesNK]

This is what gRPC's HttpContent looks like:

internal class PushStreamContent : HttpContent
{
    private readonly Func<Stream, Task> _onStreamAvailable;

    public PushStreamContent(Func<Stream, Task> onStreamAvailable, MediaTypeHeaderValue mediaType)
    {
        _onStreamAvailable = onStreamAvailable;
        Headers.ContentType = mediaType;
    }

    protected override Task SerializeToStreamAsync(Stream stream, TransportContext context)
    {
        return _onStreamAvailable(stream);
    }

    protected override bool TryComputeLength(out long length)
    {
        // We can't know the length of the content being pushed to the output stream.
        length = -1;
        return false;
    }

    // For unit tests. Hacky. ReadAsStreamAsync does not complete until SerializeToStreamAsync finishes
    internal Task PushComplete => ReadAsStreamAsync();
}

In a client or duplex streaming call gRPC will write to the stream until the client closes it (either explicitly, or by the response ending).

_writeStreamTcs = new TaskCompletionSource<Stream>(TaskCreationOptions.RunContinuationsAsynchronously);
_completeTcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

message.Content = new PushStreamContent(
    (stream) =>
    {
        _writeStreamTcs.TrySetResult(stream);
        return _completeTcs.Task;
    },
    GrpcProtocolConstants.GrpcContentTypeHeaderValue);

[Tratcher]

or by the response ending

This part that might break you. Proposal D is for the response stream not to end until the request stream ends. Looks like a classic deadlock for you.

[JamesNK]

That seems problematic. We need to read to the end of the response stream. Its completion is the only indication we have that HttpResponseMessage.TrailingHeaders has been populated with values.

I'm going to experiment with Grpc.Core (native client and server) to see what they do when the response has been completed while the client stream is still open.

[JamesNK]

Ok, here is what Grpc.Core does.

Scenario 1: Client explicitly sends the request stream, completes it, and awaits a response. The server reads the entire request stream before sending a response.

Client sends an DATA END_STREAM=true frame.

Server responds with HEADER, DATA, HEADER. The final HEADER (trailers) has END_STREAM=true.

Scenario 2: Client explicitly sends the request stream, completes it, and awaits a response. The server does not read the entire request stream before sending a response.

Client never sends a DATA END_STREAM=true frame.

Server responds with HEADER, DATA, HEADER, RST_STREAM. The final HEADER (trailers) has END_STREAM=true. RST_STREAM has NO_ERROR.

[JamesNK]

tldr: the server sends a RST_STREAM (NO_ERROR) frame to the client if it never reads an END_STREAM=true frame in the request stream.

@wfurt @stephentoub In option D, will the RST_STREAM force the client's request stream to close, which allow's the response stream to end?

@Tratcher Does Kestrel automatically send a RST_STREAM if the response finishes without reading the entire request? Is this behavior part of the standard? I worry about option D breaking HttpClient if a server implementation never resets the request stream when it finishes.

[JamesNK]

https://tools.ietf.org/html/rfc7540#section-8.1

@Tratcher Looks optional:

An HTTP response is complete after the server sends -- or the client
receives -- a frame with the END_STREAM flag set (including any
CONTINUATION frames needed to complete a header block). 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 this is true, a server MAY
request that the client abort transmission of a request without error
by sending a RST_STREAM with an error code of NO_ERROR after sending
a complete response (i.e., a frame with the END_STREAM flag).
Clients MUST NOT discard responses as a result of receiving such a
RST_STREAM, though clients can always discard responses at their
discretion for other reasons.

[Tratcher]

Yes, kestrel does Reset if the request body has not been completely read.
https://github.com/aspnet/AspNetCore/blob/04bf1bf32e13d35d4d7f61f9cbeb9bfa22e0c617/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs#L102
However, I need to confirm it doesn't try to drain the request body first:
https://github.com/aspnet/AspNetCore/blob/f652c222028ebd504203dce6a68e67b416465aab/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs#L705

[wfurt]

If HttpClient gets RST_STREAM before EOF, it will rest response stream and throw exception. We put protocol error to inner exception.

[JamesNK]

In situation 2 above the response stream has been ended with END_STREAM=true. The RST_STREAM comes afterwards to tell the client to end the request stream (if it is still open).

[davidsh]

@Tratcher @JamesNK Can you summarize where we are on this issue? What do we need to do for .NET Core 3.0 release? Is this now a documentation issue? Or do we need to change product code/behavior?

[JamesNK]

@Tratcher is the person to talk to about this.

(also, I'm on holiday beginning of this week 🌴🌞)