From 287e168aa967e0a9224042bc34eee4fffff22e68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marie=20P=C3=ADchov=C3=A1?= <11718369+ManickaP@users.noreply.github.com> Date: Wed, 9 Sep 2020 14:27:08 +0200 Subject: [PATCH] Fix exception mapping during expect 100 cancellation. (#41800) --- .../Http/SocketsHttpHandler/HttpConnection.cs | 84 +++++++++++-------- 1 file changed, 51 insertions(+), 33 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs index 47056c0a05b98d..6ca0ed87edf4c9 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs @@ -733,14 +733,22 @@ public async Task SendAsyncCore(HttpRequestMessage request, // We're awaiting the task to propagate the exception in this case. if (Volatile.Read(ref _disposed) == Status_Disposed) { - if (async) + try { - await sendRequestContentTask.ConfigureAwait(false); + if (async) + { + await sendRequestContentTask.ConfigureAwait(false); + } + else + { + // No way around it here if we want to get the exception from the task. + sendRequestContentTask.GetAwaiter().GetResult(); + } } - else + // Map the exception the same way as we normally do. + catch (Exception ex) when (MapSendException(ex, cancellationToken, out Exception mappedEx)) { - // No way around it here if we want to get the exception from the task. - sendRequestContentTask.GetAwaiter().GetResult(); + throw mappedEx; } } LogExceptions(sendRequestContentTask); @@ -751,42 +759,52 @@ public async Task SendAsyncCore(HttpRequestMessage request, // At this point, we're going to throw an exception; we just need to // determine which exception to throw. - - if (CancellationHelper.ShouldWrapInOperationCanceledException(error, cancellationToken)) - { - // Cancellation was requested, so assume that the failure is due to - // the cancellation request. This is a bit unorthodox, as usually we'd - // prioritize a non-OperationCanceledException over a cancellation - // request to avoid losing potentially pertinent information. But given - // the cancellation design where we tear down the underlying connection upon - // a cancellation request, which can then result in a myriad of different - // exceptions (argument exceptions, object disposed exceptions, socket exceptions, - // etc.), as a middle ground we treat it as cancellation, but still propagate the - // original information as the inner exception, for diagnostic purposes. - throw CancellationHelper.CreateOperationCanceledException(error, cancellationToken); - } - else if (error is InvalidOperationException) + if (MapSendException(error, cancellationToken, out Exception mappedException)) { - // For consistency with other handlers we wrap the exception in an HttpRequestException. - throw new HttpRequestException(SR.net_http_client_execution_error, error); - } - else if (error is IOException ioe) - { - // For consistency with other handlers we wrap the exception in an HttpRequestException. - // If the request is retryable, indicate that on the exception. - throw new HttpRequestException(SR.net_http_client_execution_error, ioe, _canRetry ? RequestRetryType.RetryOnSameOrNextProxy : RequestRetryType.NoRetry); - } - else - { - // Otherwise, just allow the original exception to propagate. - throw; + throw mappedException; } + // Otherwise, just allow the original exception to propagate. + throw; } } public sealed override Task SendAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken) => SendAsyncCore(request, async, cancellationToken); + private bool MapSendException(Exception exception, CancellationToken cancellationToken, out Exception mappedException) + { + if (CancellationHelper.ShouldWrapInOperationCanceledException(exception, cancellationToken)) + { + // Cancellation was requested, so assume that the failure is due to + // the cancellation request. This is a bit unorthodox, as usually we'd + // prioritize a non-OperationCanceledException over a cancellation + // request to avoid losing potentially pertinent information. But given + // the cancellation design where we tear down the underlying connection upon + // a cancellation request, which can then result in a myriad of different + // exceptions (argument exceptions, object disposed exceptions, socket exceptions, + // etc.), as a middle ground we treat it as cancellation, but still propagate the + // original information as the inner exception, for diagnostic purposes. + mappedException = CancellationHelper.CreateOperationCanceledException(exception, cancellationToken); + return true; + } + if (exception is InvalidOperationException) + { + // For consistency with other handlers we wrap the exception in an HttpRequestException. + mappedException = new HttpRequestException(SR.net_http_client_execution_error, exception); + return true; + } + if (exception is IOException ioe) + { + // For consistency with other handlers we wrap the exception in an HttpRequestException. + // If the request is retryable, indicate that on the exception. + mappedException = new HttpRequestException(SR.net_http_client_execution_error, ioe, _canRetry ? RequestRetryType.RetryOnSameOrNextProxy : RequestRetryType.NoRetry); + return true; + } + // Otherwise, just allow the original exception to propagate. + mappedException = exception; + return false; + } + private HttpContentWriteStream CreateRequestContentStream(HttpRequestMessage request) { bool requestTransferEncodingChunked = request.HasHeaders && request.Headers.TransferEncodingChunked == true;