Drain HTTP/3 response after trailers #116319
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Buffers; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
@@ -46,6 +47,9 @@ internal sealed class Http3RequestStream : IHttpStreamHeadersHandler, IAsyncDisp | |
| /// <summary>Any trailing headers.</summary> | ||
| private List<(HeaderDescriptor name, string value)>? _trailingHeaders; | ||
|
|
||
| /// <summary>Response drain task after receiving trailers.</summary> | ||
| private Task? _responseDrainTask; | ||
|
|
||
| // When reading response content, keep track of the number of bytes left in the current data frame. | ||
| private long _responseDataPayloadRemaining; | ||
|
|
||
|
|
@@ -82,56 +86,64 @@ public Http3RequestStream(HttpRequestMessage request, Http3Connection connection | |
| _responseRecvCompleted = false; | ||
| } | ||
|
|
||
| // Synchronous QuicStream.Dispose() is implemented in a sync-over-async manner, | ||
| // there is no benefit from maintaining a separate synchronous implementation in this type. | ||
| public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); | ||
|
|
||
| public async ValueTask DisposeAsync() | ||
| { | ||
| if (!_disposed) | ||
| { | ||
| _disposed = true; | ||
| Task? disposeTask = null; | ||
| if (_responseDrainTask is not null) | ||
| { | ||
| disposeTask = WaitForDrainCompletionAndDisposeAsync(); | ||
| } | ||
| else | ||
| { | ||
| AbortStream(); | ||
| if (_stream.WritesClosed.IsCompleted) | ||
| { | ||
| disposeTask = _stream.DisposeAsync().AsTask(); | ||
| } | ||
| } | ||
|
|
||
| if (disposeTask is not null) | ||
| { | ||
| _connection.LogExceptions(disposeTask); | ||
antonfirsov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| else | ||
| { | ||
| await _stream.DisposeAsync().ConfigureAwait(false); | ||
| } | ||
|
|
||
| _connection.RemoveStream(_stream); | ||
|
There was a problem hiding this comment. I'm wondering if we could hit the issue with GC eating the H3 stream while inside There was a problem hiding this comment. I see us doing the same in I must admit that I'm confused. According to my understanding of the article, the problem is that the state machine is unrooted (thus all its' references to locals and Edit: changed the comment to keep it on point There was a problem hiding this comment. @stephentoub can you please help and give some pointers what should the right approach here? There was a problem hiding this comment. I'm introducing a method here that is usually called in a fire-and-forget manner: The owning There was a problem hiding this comment. As noted in the linked blog post, async method state machine objects are kept alive by the thing they're awaiting. The question then isn't whether Http3RequestStream is rooted, but whether _responseDrainTask will eventually complete and is thus itself rooted (since the only way it could be completed is if something rooted was referencing it in order to complete it). |
||
| _sendBuffer.Dispose(); | ||
|
|
||
| // If response drain is in progress it might be still using _recvBuffer; let WaitForDrainCompletionAndDisposeAsync() dispose it. | ||
| if (_responseDrainTask is null) | ||
| { | ||
| _recvBuffer.Dispose(); | ||
| } | ||
| } | ||
|
|
||
| async Task WaitForDrainCompletionAndDisposeAsync() | ||
| { | ||
| Debug.Assert(_responseDrainTask is not null); | ||
| await _responseDrainTask.ConfigureAwait(false); | ||
| AbortStream(); | ||
| await _stream.DisposeAsync().ConfigureAwait(false); | ||
| _recvBuffer.Dispose(); | ||
| } | ||
| } | ||
|
|
||
| private void RemoveFromConnectionIfDone() | ||
| { | ||
| if (_responseRecvCompleted && _requestSendCompleted) | ||
| { | ||
| _connection.RemoveStream(_stream); | ||
| } | ||
| } | ||
antonfirsov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| public void GoAway() | ||
|
|
@@ -564,10 +576,9 @@ private async ValueTask DrainContentLength0Frames(CancellationToken cancellation | |
| _trailingHeaders = new List<(HeaderDescriptor name, string value)>(); | ||
| await ReadHeadersAsync(payloadLength, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // We do not expect more DATA frames after the trailers. | ||
| // Start draining the response to avoid aborting reads during disposal. | ||
| _responseDrainTask = DrainResponseAsync(); | ||
antonfirsov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| goto case null; | ||
| case null: | ||
| // Done receiving: copy over trailing headers. | ||
|
|
@@ -1335,6 +1346,54 @@ private void HandleReadResponseContentException(Exception ex, CancellationToken | |
| throw new HttpIOException(HttpRequestError.Unknown, SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, ex)); | ||
| } | ||
|
|
||
|
|
||
| /// <summary> | ||
| /// Drain the underlying QuicStream without attempting to interpret the data. | ||
| /// Note: this does leave us open to a bad server sending us out of order frames. | ||
| /// </summary> | ||
| private async Task DrainResponseAsync() | ||
| { | ||
| HttpConnectionSettings settings = _connection.Pool.Settings; | ||
|
There was a problem hiding this comment. You could short-circuit this with checking There was a problem hiding this comment.
|
||
| TimeSpan drainTime = settings._maxResponseDrainTime; | ||
| int remaining = settings._maxResponseDrainSize; | ||
| Debug.Assert(remaining >= 0); | ||
| if (drainTime == TimeSpan.Zero || remaining == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| using CancellationTokenSource cts = new CancellationTokenSource(settings._maxResponseDrainTime); | ||
| try | ||
| { | ||
| // If there is more data than MaxResponseDrainSize, we will silently stop draining and let Dispose(Async) abort the reads. | ||
| while (remaining > 0) | ||
| { | ||
| _recvBuffer.EnsureAvailableSpace(1); | ||
| Memory<byte> buffer = remaining >= _recvBuffer.AvailableMemory.Length ? _recvBuffer.AvailableMemory : _recvBuffer.AvailableMemory.Slice(0, remaining); | ||
| int bytesRead = await _stream.ReadAsync(buffer, cts.Token).ConfigureAwait(false); | ||
| if (bytesRead == 0) | ||
| { | ||
| // Reached EOS. | ||
| return; | ||
| } | ||
| remaining -= bytesRead; | ||
| _recvBuffer.Commit(bytesRead); | ||
| _recvBuffer.Discard(bytesRead); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Eat exceptions and stop draining to unblock QuicStream disposal waiting for response drain. | ||
| if (NetEventSource.Log.IsEnabled()) | ||
| { | ||
| string message = ex is OperationCanceledException oce && oce.CancellationToken == cts.Token ? "Response drain timed out." : $"Response drain failed with exception: {ex}"; | ||
| Trace(message); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
| } | ||
|
|
||
| private async ValueTask<bool> ReadNextDataFrameAsync(HttpResponseMessage response, CancellationToken cancellationToken) | ||
| { | ||
| if (_responseDataPayloadRemaining == -1) | ||
|
|
@@ -1365,11 +1424,10 @@ private async ValueTask<bool> ReadNextDataFrameAsync(HttpResponseMessage respons | |
| _trailingHeaders = new List<(HeaderDescriptor name, string value)>(); | ||
| await ReadHeadersAsync(payloadLength, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // We do not expect more DATA frames after the trailers. | ||
| // Start draining the response to avoid aborting reads during disposal. | ||
| _responseDrainTask = DrainResponseAsync(); | ||
|
There was a problem hiding this comment. So this change will only drain the stream if there are trailers, right? There was a problem hiding this comment.
Yes. Since #60118 talked about only this specific case, and I didn't consider others. Should we do it? If yes should it happen in this PR? There was a problem hiding this comment. Hmmm, what I'm thinking is that if it means just to just plug-in There was a problem hiding this comment.
I spent more time examining The primary issue with the trailer handling logic on This will short circuit subsequent calls to |
||
|
|
||
| goto case null; | ||
| case null: | ||
| // End of stream. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.