add support for 100-continue for H/2

[wfurt]

fixes #31312
This changes adds logic to process Expect: 100-continue request header and 10x responses.

[wfurt]

all tests are finally passing.

[stephentoub]

If this throws, what happens to potential failures in responseHeadersTask?

[wfurt]

We will call http2Stream.Dispose() bellow and that should make it fail. I'm not sure if you want to propagate the error or if you think we need to so something about it. Right now, I think we will simply throw first failure to caller.

[stephentoub]

I'm not sure if you want to propagate the error or if you think we need to so something about it.

If the body task fails, we don't need to also somehow propagate the failure in the responseHeadersTask, but that Task is almost certainly going to also fail with an exception, and if we don't do anything with it, that'll cause TaskScheduler.UnobservedTaskException to be raised. It'd be better to observe the exception in a continuation and log it... that continuation can be hooked up only in the case in a catch around the await, such that we only pay for it if we're failing anyway.

[stephentoub]

If we get here, the responseHeadersTask completed, right? We're not awaiting it to propagate any exceptions that may have occurred.

Seems like this logic could be restructured a little, e.g.

if (bodyTask == await Task.WhenAny(bodyTask, responseHeadersTask).ConfigureAwait(false) ||
    bodyTask.IsCompleted)
{
    // The sending of the request body completed before receiving all of the request headers.
    Task t = bodyTask;
    bodyTask = null;
    try
    {
        await t.ConfigureAwait(false);
    }
    catch
    {
        LogExceptionAsync(this, responseHeadersTask, "Response headers"); // ContinueWith logic factored into a helper
        throw;
    }

    await responseHeadersTask.ConfigureAwait(false);
}
else
{
    // We received the response headers but the request body hasn't yet finished.
    bodyTask.LogExceptionAsync(this, responseHeadersTask, "Request body");
}

That said, I'm not clear on the else case above where the request headers have completed but the response body hasn't. If the response body subsequently fails and we're still handling the response body, wouldn't we want to propagate any failure there out via the response body handling? If that's already completed as well, we're out of luck and logging is the best we can do, but if it hasn't yet completed, seems we'd want to take the opportunity to forward the error out?

[wfurt]

we don't have problem with ProcessIncomingFramesAsync() because we catch all exceptions, right? If so, can we do something similar and use _abortException?

[wfurt]

BTW we have few places where we have "ignored" task. Do we have same issue there?

[stephentoub]

BTW we have few places where we have "ignored" task. Do we have same issue there?

If exceptions can go unhandled in their implementations, then yes.

[stephentoub]

we don't have problem with ProcessIncomingFramesAsync() because we catch all exceptions, right?

Right.

If so, can we do something similar and use _abortException?

Quite possibly.

[wfurt]

I updated the code. I did look at what is happening with SendRequestBodyAsync(). If wee get IO exception or if server sends something we do not like (include RST and GOAWAY) we will store that in stream._abortException and we will throw if we still processing response.
I could explore that more as followup (so as ignored tasks) and we could possibly save all exceptions from SendRequestBodyAsync(). We could also log everything in there to simplify the ContinueWith()

[stephentoub]

I could explore that more as followup (so as ignored tasks) and we could possibly save all exceptions from SendRequestBodyAsync()

Yes, let's please follow-up. There are two concerns:

1. That if an error occurs from sending we don't lose the exception if at all possible. That means best case propagating it to the response and worst case logging it.
2. That we don't end up with Faulted Tasks that never have their Exceptions observed, which will cause noise for anyone listening to TaskScheduler.UnobservedTaskExceptions.

[stephentoub]

How do we know that the ObjectDisposedExceptions we're eating here are being of abandoning sending a request?

[wfurt]

_shouldSendRequestBody be set to false when we get deny (300+) response code. Originally that was done only for 100Continue but it got changed. Assuming that sending more body will not change server's decision

[wfurt]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s), but failed to run 2 pipeline(s).

[wfurt]

I executed streaming gRPC tests (and few others as well) and it looks OK.

[wtgodbe]

The test timeout was bogus, the tests completed but never reported back to AzDO. Second or third time I've seen this today (#38099 (comment)). We should be good to merge this - @wfurt any last concerns?

[stephentoub]

Let's go ahead and get this in and we can address additional clean-up / fix-up subsequently.

[geoffkizer]

@stephentoub @wfurt is there an issue for the "additional cleanup" needed here?

[wfurt]

#36855 is still open @geoffkizer and I plan to use it to close raised issues. I just posed #38226 to do what I discussed with @stephentoub