Log exceptions from asynchronously failing Expect: 100-continue sends (#37391)

* Log exceptions from asynchronously failing Expect: 100-continue sends

When Expect: 100-continue is used, the sending of the request body content is allowed to run concurrently with the receipt of the response headers.  If the processing of the response headers encounters an exception, we currently fail to ever join with that send task, which results in a TaskScheduler.UnobservedTaskException event.  This PR changes to observe the exception in such cases and log it.

(In doing so, I also came across an async void method, and changed all of the remaining ones in the assembly to be async Task instead.  The current implementation of async void isn't any cheaper than async Task, and is actually more expensive in certain ways, plus it unnecessarily interacts with any SynchronizationContext that may exist.)

* Address PR feedback

(cherry picked from commit 0bc3ee4a4f)
This commit is contained in:
Stephen Toub 2019-05-03 17:33:55 -04:00 коммит произвёл Marek Safar
Родитель 15b2cedee1
Коммит eb5ba09e18
3 изменённых файлов: 36 добавлений и 8 удалений

Просмотреть файл

@ -554,7 +554,7 @@ namespace System.Net.Http
Task.Factory.StartNew( Task.Factory.StartNew(
s => { s => {
var whrs = (WinHttpRequestState)s; var whrs = (WinHttpRequestState)s;
whrs.Handler.StartRequest(whrs); _ = whrs.Handler.StartRequestAsync(whrs);
}, },
state, state,
CancellationToken.None, CancellationToken.None,
@ -764,7 +764,7 @@ namespace System.Net.Http
} }
} }
private async void StartRequest(WinHttpRequestState state) private async Task StartRequestAsync(WinHttpRequestState state)
{ {
if (state.CancellationToken.IsCancellationRequested) if (state.CancellationToken.IsCancellationRequested)
{ {

Просмотреть файл

@ -123,18 +123,31 @@ namespace System.Net.Http
ValueTask<int>? readAheadTask = ConsumeReadAheadTask(); ValueTask<int>? readAheadTask = ConsumeReadAheadTask();
if (readAheadTask != null) if (readAheadTask != null)
{ {
IgnoreExceptionsAsync(readAheadTask.GetValueOrDefault()); _ = IgnoreExceptionsAsync(readAheadTask.GetValueOrDefault());
} }
} }
} }
} }
/// <summary>Awaits a task, ignoring any resulting exceptions.</summary> /// <summary>Awaits a task, ignoring any resulting exceptions.</summary>
private static async void IgnoreExceptionsAsync(ValueTask<int> task) private static async Task IgnoreExceptionsAsync(ValueTask<int> task)
{ {
try { await task.ConfigureAwait(false); } catch { } try { await task.ConfigureAwait(false); } catch { }
} }
/// <summary>Awaits a task, logging any resulting exceptions (which are otherwise ignored).</summary>
private async Task LogExceptionsAsync(Task task)
{
try
{
await task.ConfigureAwait(false);
}
catch (Exception e)
{
if (NetEventSource.IsEnabled) Trace($"Exception from asynchronous processing: {e}");
}
}
/// <summary>Do a non-blocking poll to see whether the connection has data available or has been closed.</summary> /// <summary>Do a non-blocking poll to see whether the connection has data available or has been closed.</summary>
/// <remarks>If we don't have direct access to the underlying socket, we instead use a read-ahead task.</remarks> /// <remarks>If we don't have direct access to the underlying socket, we instead use a read-ahead task.</remarks>
public bool PollRead() public bool PollRead()
@ -356,6 +369,7 @@ namespace System.Net.Http
public async Task<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken) public async Task<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
{ {
TaskCompletionSource<bool> allowExpect100ToContinue = null; TaskCompletionSource<bool> allowExpect100ToContinue = null;
Task sendRequestContentTask = null;
Debug.Assert(_currentRequest == null, $"Expected null {nameof(_currentRequest)}."); Debug.Assert(_currentRequest == null, $"Expected null {nameof(_currentRequest)}.");
Debug.Assert(RemainingBuffer.Length == 0, "Unexpected data in read buffer"); Debug.Assert(RemainingBuffer.Length == 0, "Unexpected data in read buffer");
@ -462,7 +476,6 @@ namespace System.Net.Http
// CRLF for end of headers. // CRLF for end of headers.
await WriteTwoBytesAsync((byte)'\r', (byte)'\n').ConfigureAwait(false); await WriteTwoBytesAsync((byte)'\r', (byte)'\n').ConfigureAwait(false);
Task sendRequestContentTask = null;
if (request.Content == null) if (request.Content == null)
{ {
// We have nothing more to send, so flush out any headers we haven't yet sent. // We have nothing more to send, so flush out any headers we haven't yet sent.
@ -574,8 +587,9 @@ namespace System.Net.Http
// content has been received, so this task should generally already be complete. // content has been received, so this task should generally already be complete.
if (sendRequestContentTask != null) if (sendRequestContentTask != null)
{ {
await sendRequestContentTask.ConfigureAwait(false); Task sendTask = sendRequestContentTask;
sendRequestContentTask = null; sendRequestContentTask = null;
await sendTask.ConfigureAwait(false);
} }
// Parse the response headers. // Parse the response headers.
@ -665,6 +679,20 @@ namespace System.Net.Http
allowExpect100ToContinue?.TrySetResult(false); allowExpect100ToContinue?.TrySetResult(false);
if (NetEventSource.IsEnabled) Trace($"Error sending request: {error}"); if (NetEventSource.IsEnabled) Trace($"Error sending request: {error}");
// In the rare case where Expect: 100-continue was used and then processing
// of the response headers encountered an error such that we weren't able to
// wait for the sending to complete, it's possible the sending also encountered
// an exception or potentially is still going and will encounter an exception
// (we're about to Dispose for the connection). In such cases, we don't want any
// exception in that sending task to become unobserved and raise alarm bells, so we
// hook up a continuation that will log it.
if (sendRequestContentTask != null && !sendRequestContentTask.IsCompletedSuccessfully)
{
_ = LogExceptionsAsync(sendRequestContentTask);
}
// Now clean up the connection.
Dispose(); Dispose();
// At this point, we're going to throw an exception; we just need to // At this point, we're going to throw an exception; we just need to

Просмотреть файл

@ -74,14 +74,14 @@ namespace System.Net.Http
// Start the asynchronous drain. // Start the asynchronous drain.
// It may complete synchronously, in which case the connection will be put back in the pool synchronously. // It may complete synchronously, in which case the connection will be put back in the pool synchronously.
// Skip the call to base.Dispose -- it will be deferred until DrainOnDisposeAsync finishes. // Skip the call to base.Dispose -- it will be deferred until DrainOnDisposeAsync finishes.
DrainOnDisposeAsync(); _ = DrainOnDisposeAsync();
return; return;
} }
base.Dispose(disposing); base.Dispose(disposing);
} }
private async void DrainOnDisposeAsync() private async Task DrainOnDisposeAsync()
{ {
HttpConnection connection = _connection; // Will be null after drain succeeds HttpConnection connection = _connection; // Will be null after drain succeeds