Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exception mapping during expect 100 cancellation. #41800

Merged
merged 3 commits into from
Sep 9, 2020
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -733,14 +733,26 @@ public async Task<HttpResponseMessage> 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();
// Map the exception the same way as we normally do.
if (MapSendException(ex, cancellationToken, out Exception mappedEx))
{
throw mappedEx;
}
throw;
ManickaP marked this conversation as resolved.
Show resolved Hide resolved
}
}
LogExceptions(sendRequestContentTask);
Expand All @@ -751,42 +763,52 @@ public async Task<HttpResponseMessage> 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<HttpResponseMessage> 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;
Expand Down