From e004b25287a5b59279b15d5bf03ebd8904395177 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marie=20P=C3=ADchov=C3=A1?= Date: Thu, 3 Sep 2020 16:14:20 +0200 Subject: [PATCH 1/3] Fix exception mapping during expect 100 cancellation. --- .../Http/SocketsHttpHandler/HttpConnection.cs | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 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 47056c0a05b98..59d07dd03fa72 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,17 +733,29 @@ 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 + catch (Exception ex) { - // No way around it here if we want to get the exception from the task. - sendRequestContentTask.GetAwaiter().GetResult(); + // Overwrite the exception so that the one thrown by sendRequestContentTask one gets propagated, + // but still gets handled the same way as any other. + error = ex; } } - LogExceptions(sendRequestContentTask); + else + { + LogExceptions(sendRequestContentTask); + } } // Now clean up the connection. From 430015e2f1bbf6035a2db960c8eb40b316c1775d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marie=20P=C3=ADchov=C3=A1?= Date: Thu, 3 Sep 2020 20:07:01 +0200 Subject: [PATCH 2/3] Fixed exception mapping. --- .../Http/SocketsHttpHandler/HttpConnection.cs | 80 +++++++++++-------- 1 file changed, 45 insertions(+), 35 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 59d07dd03fa72..6430014b1eae5 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 @@ -747,15 +747,15 @@ public async Task SendAsyncCore(HttpRequestMessage request, } catch (Exception ex) { - // Overwrite the exception so that the one thrown by sendRequestContentTask one gets propagated, - // but still gets handled the same way as any other. - error = ex; + // Map the exception the same way as we normally do. + if (MapSendException(ex, cancellationToken, out Exception mappedEx)) + { + throw mappedEx; + } + throw; } } - else - { - LogExceptions(sendRequestContentTask); - } + LogExceptions(sendRequestContentTask); } // Now clean up the connection. @@ -763,42 +763,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; From 0a28df5f17dee3a6bf2e1f9c9b38e99f06691fcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marie=20P=C3=ADchov=C3=A1?= Date: Wed, 9 Sep 2020 11:52:51 +0200 Subject: [PATCH 3/3] Addressed feedback. --- .../Net/Http/SocketsHttpHandler/HttpConnection.cs | 10 +++------- 1 file changed, 3 insertions(+), 7 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 6430014b1eae5..6ca0ed87edf4c 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 @@ -745,14 +745,10 @@ public async Task SendAsyncCore(HttpRequestMessage request, sendRequestContentTask.GetAwaiter().GetResult(); } } - catch (Exception ex) + // Map the exception the same way as we normally do. + catch (Exception ex) when (MapSendException(ex, cancellationToken, out Exception mappedEx)) { - // Map the exception the same way as we normally do. - if (MapSendException(ex, cancellationToken, out Exception mappedEx)) - { - throw mappedEx; - } - throw; + throw mappedEx; } } LogExceptions(sendRequestContentTask);