Skip to content

React to HttpRequestError API changes #89124

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 18, 2023
Merged

React to HttpRequestError API changes #89124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b4dcf2
React to HttpRequestError API changes
MihaZupan Jul 18, 2023
2bee08f
Use InvalidResponse for net_http_invalid_response_multiple_status_codes
MihaZupan Jul 18, 2023
eee6bba
Use HttpRequestError.Unknown for the rest
MihaZupan Jul 18, 2023
0edf122
Add simple tests for the HttpRequestError property
MihaZupan Jul 18, 2023
6f64e22
Fix the inner exception being removed by mistake
MihaZupan Jul 18, 2023
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
4 changes: 2 additions & 2 deletions src/libraries/System.Net.Http/ref/System.Net.Http.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,8 @@ public HttpRequestException() { }
public HttpRequestException(string? message) { }
public HttpRequestException(string? message, System.Exception? inner) { }
public HttpRequestException(string? message, System.Exception? inner, System.Net.HttpStatusCode? statusCode) { }
public HttpRequestException(string? message, System.Exception? inner = null, System.Net.HttpStatusCode? statusCode = null, System.Net.Http.HttpRequestError? httpRequestError = null) { }
public System.Net.Http.HttpRequestError? HttpRequestError { get { throw null; } }
public HttpRequestException(System.Net.Http.HttpRequestError httpRequestError, string? message = null, System.Exception? inner = null, System.Net.HttpStatusCode? statusCode = null) { }
public System.Net.Http.HttpRequestError HttpRequestError { get { throw null; } }
public System.Net.HttpStatusCode? StatusCode { get { throw null; } }
}
public partial class HttpRequestMessage : System.IDisposable
Expand Down
7 changes: 4 additions & 3 deletions src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Buffers;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
using System.Net.Http.Headers;
using System.Text;
Expand Down Expand Up @@ -608,7 +609,7 @@ private bool CreateTemporaryBuffer(long maxBufferSize, out MemoryStream? tempBuf
// This should only be hit when called directly; HttpClient/HttpClientHandler
// will not exceed this limit.
throw new ArgumentOutOfRangeException(nameof(maxBufferSize), maxBufferSize,
SR.Format(System.Globalization.CultureInfo.InvariantCulture,
SR.Format(CultureInfo.InvariantCulture,
SR.net_http_content_buffersize_limit, HttpContent.MaxBufferSize));
}

Expand Down Expand Up @@ -720,7 +721,7 @@ internal static Exception WrapStreamCopyException(Exception e)
{
Debug.Assert(StreamCopyExceptionNeedsWrapping(e));
HttpRequestError error = e is HttpIOException ioEx ? ioEx.HttpRequestError : HttpRequestError.Unknown;
return new HttpRequestException(SR.net_http_content_stream_copy_error, e, httpRequestError: error);
return new HttpRequestException(error, SR.net_http_content_stream_copy_error, e);
}

private static int GetPreambleLength(ArraySegment<byte> buffer, Encoding encoding)
Expand Down Expand Up @@ -835,7 +836,7 @@ private static async Task<TResult> WaitAndReturnAsync<TState, TResult>(Task wait

private static HttpRequestException CreateOverCapacityException(long maxBufferSize)
{
return new HttpRequestException(SR.Format(System.Globalization.CultureInfo.InvariantCulture, SR.net_http_content_buffersize_exceeded, maxBufferSize), httpRequestError: HttpRequestError.ConfigurationLimitExceeded);
return new HttpRequestException(HttpRequestError.ConfigurationLimitExceeded, SR.Format(CultureInfo.InvariantCulture, SR.net_http_content_buffersize_exceeded, maxBufferSize));
}

internal sealed class LimitMemoryStream : MemoryStream
Expand Down
13 changes: 5 additions & 8 deletions src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ public HttpRequestException(string? message, Exception? inner, HttpStatusCode? s
/// <summary>
/// Initializes a new instance of the <see cref="HttpRequestException" /> class with a specific message an inner exception, and an HTTP status code and an <see cref="HttpRequestError"/>.
/// </summary>
/// <param name="httpRequestError">The <see cref="HttpRequestError"/> that caused the exception.</param>
/// <param name="message">A message that describes the current exception.</param>
/// <param name="inner">The inner exception.</param>
/// <param name="statusCode">The HTTP status code.</param>
/// <param name="httpRequestError">The <see cref="HttpRequestError"/> that caused the exception.</param>
public HttpRequestException(string? message, Exception? inner = null, HttpStatusCode? statusCode = null, HttpRequestError? httpRequestError = null)
public HttpRequestException(HttpRequestError httpRequestError, string? message = null, Exception? inner = null, HttpStatusCode? statusCode = null)
: this(message, inner, statusCode)
{
HttpRequestError = httpRequestError;
Expand All @@ -51,10 +51,7 @@ public HttpRequestException(string? message, Exception? inner = null, HttpStatus
/// <summary>
/// Gets the <see cref="Http.HttpRequestError"/> that caused the exception.
/// </summary>
/// <value>
/// The <see cref="Http.HttpRequestError"/> or <see langword="null"/> if the underlying <see cref="HttpMessageHandler"/> did not provide it.
/// </value>
public HttpRequestError? HttpRequestError { get; }
public HttpRequestError HttpRequestError { get; }

/// <summary>
/// Gets the HTTP status code to be returned with the exception.
Expand All @@ -66,8 +63,8 @@ public HttpRequestException(string? message, Exception? inner = null, HttpStatus

// This constructor is used internally to indicate that a request was not successfully sent due to an IOException,
// and the exception occurred early enough so that the request may be retried on another connection.
internal HttpRequestException(string? message, Exception? inner, RequestRetryType allowRetry, HttpRequestError? httpRequestError = null)
: this(message, inner, httpRequestError: httpRequestError)
internal HttpRequestException(HttpRequestError httpRequestError, string? message, Exception? inner, RequestRetryType allowRetry)
: this(httpRequestError, message, inner)
{
AllowRetry = allowRetry;
}
Expand Down
2 changes: 1 addition & 1 deletion ...ies/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ private static async Task<HttpResponseMessage> SendWithNtAuthAsync(HttpRequestMe
{
isNewConnection = false;
connection.Dispose();
throw new HttpRequestException(SR.Format(SR.net_http_authvalidationfailure, statusCode), null, HttpStatusCode.Unauthorized, HttpRequestError.UserAuthenticationError);
throw new HttpRequestException(HttpRequestError.UserAuthenticationError, SR.Format(SR.net_http_authvalidationfailure, statusCode), statusCode: HttpStatusCode.Unauthorized);
}
break;
}
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public static async ValueTask<SslStream> EstablishSslConnectionAsync(SslClientAu
throw CancellationHelper.CreateOperationCanceledException(e, cancellationToken);
}

HttpRequestException ex = new HttpRequestException(SR.net_http_ssl_connection_failed, e, httpRequestError: HttpRequestError.SecureConnectionError);
HttpRequestException ex = new HttpRequestException(HttpRequestError.SecureConnectionError, SR.net_http_ssl_connection_failed, e);
if (request.IsExtendedConnectRequest)
{
// Extended connect request is negotiating strictly for ALPN = "h2" because HttpClient is unaware of a possible downgrade.
Expand Down Expand Up @@ -139,7 +139,7 @@ internal static Exception CreateWrappedException(Exception exception, string hos
{
return CancellationHelper.ShouldWrapInOperationCanceledException(exception, cancellationToken) ?
CancellationHelper.CreateOperationCanceledException(exception, cancellationToken) :
new HttpRequestException($"{exception.Message} ({host}:{port})", exception, RequestRetryType.RetryOnNextProxy, DeduceError(exception));
new HttpRequestException(DeduceError(exception), $"{exception.Message} ({host}:{port})", exception, RequestRetryType.RetryOnNextProxy);

static HttpRequestError DeduceError(Exception exception)
{
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2104,11 +2104,11 @@ await Task.WhenAny(requestBodyTask, responseHeadersTask).ConfigureAwait(false) =
}
catch (HttpIOException e)
{
throw new HttpRequestException(e.Message, e, httpRequestError: e.HttpRequestError);
throw new HttpRequestException(e.HttpRequestError, e.Message, e);
}
catch (Exception e) when (e is IOException || e is ObjectDisposedException || e is InvalidOperationException)
{
throw new HttpRequestException(SR.net_http_client_execution_error, e, httpRequestError: HttpRequestError.Unknown);
throw new HttpRequestException(HttpRequestError.Unknown, SR.net_http_client_execution_error, e);
}
}

Expand Down Expand Up @@ -2205,7 +2205,7 @@ internal void Trace(int streamId, string message, [CallerMemberName] string? mem

[DoesNotReturn]
private static void ThrowRetry(string message, Exception? innerException = null) =>
throw new HttpRequestException(message, innerException, allowRetry: RequestRetryType.RetryOnConnectionFailure);
throw new HttpRequestException(HttpRequestError.Unknown, message, innerException, RequestRetryType.RetryOnConnectionFailure);

private static Exception GetRequestAbortedException(Exception? innerException = null) =>
innerException as HttpIOException ?? new IOException(SR.net_http_request_aborted, innerException);
Expand Down
16 changes: 8 additions & 8 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ void IHttpStreamHeadersHandler.OnStaticIndexedHeader(int index)
if (index <= LastHPackRequestPseudoHeaderId)
{
if (NetEventSource.Log.IsEnabled()) Trace($"Invalid request pseudo-header ID {index}.");
throw new HttpRequestException(SR.net_http_invalid_response, httpRequestError: HttpRequestError.InvalidResponse);
throw new HttpRequestException(HttpRequestError.InvalidResponse, SR.net_http_invalid_response);
}
else if (index <= LastHPackStatusPseudoHeaderId)
{
Expand All @@ -563,7 +563,7 @@ void IHttpStreamHeadersHandler.OnStaticIndexedHeader(int index, ReadOnlySpan<byt
if (index <= LastHPackRequestPseudoHeaderId)
{
if (NetEventSource.Log.IsEnabled()) Trace($"Invalid request pseudo-header ID {index}.");
throw new HttpRequestException(SR.net_http_invalid_response, httpRequestError: HttpRequestError.InvalidResponse);
throw new HttpRequestException(HttpRequestError.InvalidResponse, SR.net_http_invalid_response);
}
else if (index <= LastHPackStatusPseudoHeaderId)
{
Expand All @@ -589,7 +589,7 @@ private void AdjustHeaderBudget(int amount)
_headerBudgetRemaining -= amount;
if (_headerBudgetRemaining < 0)
{
throw new HttpRequestException(SR.Format(SR.net_http_response_headers_exceeded_length, _connection._pool.Settings.MaxResponseHeadersByteLength), httpRequestError: HttpRequestError.ConfigurationLimitExceeded);
throw new HttpRequestException(HttpRequestError.ConfigurationLimitExceeded, SR.Format(SR.net_http_response_headers_exceeded_length, _connection._pool.Settings.MaxResponseHeadersByteLength));
}
}

Expand All @@ -611,14 +611,14 @@ private void OnStatus(int statusCode)
if (_responseProtocolState == ResponseProtocolState.ExpectingHeaders)
{
if (NetEventSource.Log.IsEnabled()) Trace("Received extra status header.");
throw new HttpRequestException(SR.net_http_invalid_response_multiple_status_codes, httpRequestError: HttpRequestError.ConfigurationLimitExceeded);
throw new HttpRequestException(HttpRequestError.InvalidResponse, SR.net_http_invalid_response_multiple_status_codes);
}

if (_responseProtocolState != ResponseProtocolState.ExpectingStatus)
{
// Pseudo-headers are allowed only in header block
if (NetEventSource.Log.IsEnabled()) Trace($"Status pseudo-header received in {_responseProtocolState} state.");
throw new HttpRequestException(SR.net_http_invalid_response_pseudo_header_in_trailer, httpRequestError: HttpRequestError.InvalidResponse);
throw new HttpRequestException(HttpRequestError.InvalidResponse, SR.net_http_invalid_response_pseudo_header_in_trailer);
}

Debug.Assert(_response != null);
Expand Down Expand Up @@ -681,7 +681,7 @@ private void OnHeader(HeaderDescriptor descriptor, ReadOnlySpan<byte> value)
if (_responseProtocolState != ResponseProtocolState.ExpectingHeaders && _responseProtocolState != ResponseProtocolState.ExpectingTrailingHeaders)
{
if (NetEventSource.Log.IsEnabled()) Trace("Received header before status.");
throw new HttpRequestException(SR.net_http_invalid_response, httpRequestError: HttpRequestError.InvalidResponse);
throw new HttpRequestException(HttpRequestError.InvalidResponse, SR.net_http_invalid_response);
}

Encoding? valueEncoding = _connection._pool.Settings._responseHeaderEncodingSelector?.Invoke(descriptor.Name, _request);
Expand Down Expand Up @@ -725,7 +725,7 @@ public void OnHeader(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
else
{
if (NetEventSource.Log.IsEnabled()) Trace($"Invalid response pseudo-header '{Encoding.ASCII.GetString(name)}'.");
throw new HttpRequestException(SR.net_http_invalid_response, httpRequestError: HttpRequestError.InvalidResponse);
throw new HttpRequestException(HttpRequestError.InvalidResponse, SR.net_http_invalid_response);
}
}
else
Expand All @@ -734,7 +734,7 @@ public void OnHeader(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
if (!HeaderDescriptor.TryGet(name, out HeaderDescriptor descriptor))
{
// Invalid header name
throw new HttpRequestException(SR.Format(SR.net_http_invalid_response_header_name, Encoding.ASCII.GetString(name)), httpRequestError: HttpRequestError.InvalidResponse);
throw new HttpRequestException(HttpRequestError.InvalidResponse, SR.Format(SR.net_http_invalid_response_header_name, Encoding.ASCII.GetString(name)));
}

OnHeader(descriptor, value);
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, lon

if (quicStream == null)
{
throw new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure);
throw new HttpRequestException(HttpRequestError.Unknown, SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure);
Copy link
Member

@stephentoub stephentoub Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not specific to this PR, but I'm surprised some of these are "Unknown". Are we missing some kind of "UserRequested" / "Canceled" / "Aborted" / etc. enum status?

Copy link
Member Author

@MihaZupan MihaZupan Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering whether some of these should be ConnectionError, but with the description of "A transport-level failure occurred while connecting to the remote endpoint.", it doesn't feel like the right fit.

A bunch of these are on retriable exceptions so in most cases the user won't see them, but it does feel like we're missing something.

}

requestStream!.StreamId = quicStream.Id;
Expand All @@ -221,7 +221,7 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, lon

if (goAway)
{
throw new HttpRequestException(SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure);
throw new HttpRequestException(HttpRequestError.Unknown, SR.net_http_request_aborted, null, RequestRetryType.RetryOnConnectionFailure);
}

if (NetEventSource.Log.IsEnabled()) Trace($"Sending request: {request}");
Expand All @@ -237,7 +237,7 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, lon
{
// This will happen if we aborted _connection somewhere and we have pending OpenOutboundStreamAsync call.
// note that _abortException may be null if we closed the connection in response to a GOAWAY frame
throw new HttpRequestException(SR.net_http_client_execution_error, _abortException, RequestRetryType.RetryOnConnectionFailure);
throw new HttpRequestException(HttpRequestError.Unknown, SR.net_http_client_execution_error, _abortException, RequestRetryType.RetryOnConnectionFailure);
}
finally
{
Expand Down
Loading