-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Implement HttpRequestError #88974
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
Implement HttpRequestError #88974
Changes from all commits
8cad616
511e16c
12e67e7
633d632
9afb790
c7ac73b
2eb3a4a
24dd331
b54e8c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.IO; | ||
|
||
namespace System.Net.Http | ||
{ | ||
/// <summary> | ||
/// An exception thrown when an error occurs while reading the response. | ||
/// </summary> | ||
public class HttpIOException : IOException | ||
{ | ||
/// <summary> | ||
/// Initializes a new instance of the <see cref="HttpIOException"/> class. | ||
/// </summary> | ||
/// <param name="httpRequestError">The <see cref="Http.HttpRequestError"/> that caused the exception.</param> | ||
/// <param name="message">The message string describing the error.</param> | ||
/// <param name="innerException">The exception that is the cause of the current exception.</param> | ||
public HttpIOException(HttpRequestError httpRequestError, string? message = null, Exception? innerException = null) | ||
: base(message, innerException) | ||
{ | ||
HttpRequestError = httpRequestError; | ||
} | ||
|
||
/// <summary> | ||
/// Gets the <see cref="Http.HttpRequestError"/> that caused the exception. | ||
/// </summary> | ||
public HttpRequestError HttpRequestError { get; } | ||
|
||
/// <inheritdoc /> | ||
public override string Message => $"{base.Message} ({HttpRequestError})"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace System.Net.Http | ||
{ | ||
/// <summary> | ||
/// Defines error categories representing the reason for <see cref="HttpRequestException"/> or <see cref="HttpIOException"/>. | ||
/// </summary> | ||
public enum HttpRequestError | ||
{ | ||
/// <summary> | ||
/// A generic or unknown error occurred. | ||
/// </summary> | ||
Unknown = 0, | ||
|
||
/// <summary> | ||
/// The DNS name resolution failed. | ||
/// </summary> | ||
NameResolutionError, | ||
|
||
/// <summary> | ||
/// A transport-level failure occurred while connecting to the remote endpoint. | ||
/// </summary> | ||
ConnectionError, | ||
|
||
/// <summary> | ||
/// An error occurred during the TLS handshake. | ||
/// </summary> | ||
SecureConnectionError, | ||
|
||
/// <summary> | ||
/// An HTTP/2 or HTTP/3 protocol error occurred. | ||
/// </summary> | ||
HttpProtocolError, | ||
|
||
/// <summary> | ||
/// Extended CONNECT for WebSockets over HTTP/2 is not supported by the peer. | ||
/// </summary> | ||
ExtendedConnectNotSupported, | ||
|
||
/// <summary> | ||
/// Cannot negotiate the HTTP Version requested. | ||
/// </summary> | ||
VersionNegotiationError, | ||
|
||
/// <summary> | ||
/// The authentication failed. | ||
/// </summary> | ||
UserAuthenticationError, | ||
|
||
/// <summary> | ||
/// An error occurred while establishing a connection to the proxy tunnel. | ||
/// </summary> | ||
ProxyTunnelError, | ||
|
||
/// <summary> | ||
/// An invalid or malformed response has been received. | ||
/// </summary> | ||
InvalidResponse, | ||
|
||
/// <summary> | ||
/// The response ended prematurely. | ||
/// </summary> | ||
ResponseEnded, | ||
|
||
/// <summary> | ||
/// The response exceeded a pre-configured limit such as <see cref="HttpClient.MaxResponseContentBufferSize"/> or <see cref="HttpClientHandler.MaxResponseHeadersLength"/>. | ||
/// </summary> | ||
ConfigurationLimitExceeded, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,17 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Diagnostics.CodeAnalysis; | ||
using System.IO; | ||
|
||
namespace System.Net.Http | ||
{ | ||
public class HttpRequestException : Exception | ||
{ | ||
internal RequestRetryType AllowRetry { get; } = RequestRetryType.NoRetry; | ||
|
||
public HttpRequestException() | ||
: this(null, null) | ||
{ } | ||
|
||
public HttpRequestException(string? message) | ||
: this(message, null) | ||
: base(message) | ||
{ } | ||
|
||
public HttpRequestException(string? message, Exception? inner) | ||
|
@@ -39,6 +35,27 @@ public HttpRequestException(string? message, Exception? inner, HttpStatusCode? s | |
StatusCode = statusCode; | ||
} | ||
|
||
/// <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="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) | ||
: this(message, inner, statusCode) | ||
{ | ||
HttpRequestError = httpRequestError; | ||
} | ||
|
||
/// <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; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this nullable if we have Unknown, or the other way around? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IMHO making difference between the two is very impractical and can be a source of problems, but that's where this ended after reacting to concerns in #76644 (comment) by removing nullability and defining a default My recommendation would be to have a follow-up discussion on this and potentially remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't understand why nullable makes sense here. I was away for the API review, and see the comments in the issue:
but I agree that this is a confusing distinction to try to make. What action is someone supposed to take differently based on null vs Unknown? You get an exception, and if an error code couldn't be produced, for whatever reason, it makes sense that it's "unknown". I don't know what I'd do differently between a null error code and an Unknown error code other than have to write more code and be confused. cc: @bartonjs, @terrajobst There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main distinction that I can think of would be "I should try again if the error is [some specific code(s)], and not if not, and Unknown is clearly not one of those codes". If it's null (vs Unknown) then the caller can decide "the underlying provider doesn't know how to set this code" is different from "something went wrong that the caller didn't know how to map to this enum" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't understand the distinction / how someone would apply it. You're saying someone would write code that would retry on null but not on Unknown, or retry on Unknown but not null? Whether the provider knows what the problem is but can't map it, or doesn't know and can't map it, the result is the same: the error is unknown and you as the consumer have zero additional information to inform you as to what to do with it. I'm not understanding why we'd choose two different representations for that same case. If there's an additional error condition that can't be mapped to one in our enum, I'd think the right answer would be to augment the enum with the additional values for those conditions. |
||
|
||
/// <summary> | ||
/// Gets the HTTP status code to be returned with the exception. | ||
/// </summary> | ||
|
@@ -49,8 +66,8 @@ public HttpRequestException(string? message, Exception? inner, HttpStatusCode? s | |
|
||
// 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) | ||
: this(message, inner) | ||
internal HttpRequestException(string? message, Exception? inner, RequestRetryType allowRetry, HttpRequestError? httpRequestError = null) | ||
: this(message, inner, httpRequestError: httpRequestError) | ||
{ | ||
AllowRetry = allowRetry; | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.