dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.19k stars 4.72k forks source link

[API Proposal]: Support error codes for HttpRequestException #76644

Closed searchmon closed 1 year ago

searchmon commented 2 years ago

Edited by @antonfirsov Update 1: changes based on feedback - https://github.com/dotnet/runtime/issues/76644#issuecomment-1525754867

Background and motivation

HttpRequestException is thrown under various conditions but it does not provide an error code to differentiate those conditions. Currently, it provides 'string Message' property inherited from Exception type and it can contain various error messages:

The above strings are defined in https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/src/Resources/Strings.resx

If we want to pragmatically handle exceptions based on the cause, it would be better dealing with error codes rather than dealing with string values. For example, when we create a dashboard for different exceptions encountered, aggregating by error codes would make more sense that doing by string values. And we could categorize some errors into a group for a special handling.

Many other exceptions like SocketException and Win32Exception have an error code field already. Would it make sense to provide a new property for HttpRequestException to hold error codes?

API Proposal


public class HttpRequestException : Exception
{
    // -- New properties --
    public HttpRequestError HttpRequestError { get; }

    // -- New constructors --
    public HttpRequestException(HttpRequestError httpRequestError, string? message) {}
    public HttpRequestException(HttpRequestError httpRequestError, string? message, Exception? inner) {}
    public HttpRequestException(HttpRequestError httpRequestError, string? message, Exception? inner, HttpStatusCode? statusCode) {}
    // ALTERNATIVE:
    public HttpRequestException(HttpRequestError httpRequestError, string? message, Exception? inner = null, HttpStatusCode? statusCode = null) {}
}

// IOException subtype to throw from response read streams
public class HttpIOException : IOException  // ALTERNATIVE name: HttpResponseReadException
{
    public HttpRequestError HttpRequestError { get; }
    public HttpResponseReadException(HttpRequestError httpRequestError, string? message = null, Exception? innerException = null) {}
} 

// The EXISTING HttpProtocolException should be the subtype of this new exception type
public class HttpProtocolException : HttpResponseReadException // WAS: IOException
{
}

// Defines disjoint error categories with high granularity.
public enum HttpRequestError
{
    Undefined = 0,                          // Uncategorized/generic error -OR-
                                            // the underlying HttpMessageHandler does not implement HttpRequestError

    NameResolutionError,                    // DNS request failed
    ConnectionError,                        // Transport-level error during connection
    TransportError,                         // Transport-level error after connection
    SecureConnectionError,                  // SSL/TLS error
    HttpProtocolError,                      // HTTP 2.0/3.0 protocol error occurred
    UnsupportedExtendedConnect,             // Extended CONNECT for WebSockets over HTTP/2 is not supported.
                                            // (SETTINGS_ENABLE_CONNECT_PROTOCOL has not been sent).
    VersionNegotiationError,                // Cannot negotiate the HTTP Version requested
    UserAuthenticationError,                // Authentication failed with the provided credentials
    ProxyTunnelError,

    StatusNotSuccess,                       // Set by EnsureSuccessStatusCode() in case of non-success status code.
    InvalidResponse,                        // General error in response/malformed response

    // ALTERNATIVE name: InvalidResponseHeaders
    InvalidResponseHeader,                  // Error with response headers

    // OPTIONAL TO NOT INCLUDE: The one's below are specific cases of errors with the response.
    // We may consider merging them into "InvalidResponse" and "InvalidResponseHeader"
    ResponseEnded,                          // Received EOF
    // These depend on SocketsHttpHandler configuration
    ContentBufferSizeExceeded,              // Response Content size exceeded MaxResponseContentBufferSize
    ResponseHeaderExceededLengthLimit,      // Response Header length exceeded MaxResponseHeadersLength
}

API Usage

With HttpClient methods:

try
{
    using var response = await httpClient.GetStringAsync(url);
}
catch (HttpRequestException ex) when (ex.HttpRequestError is HttpRequestError.ConnectionError or
                                                             HttpRequestError.SecureConnectionError or
                                                             HttpRequestError.NameResolutionError)
{
    // Retry
}

With response read Stream:

using var response = await httpClient.GetAsync(url, HttpCompletionOption.ResponseHeadersRead);
using var responseStream = await response.Content.ReadAsStreamAsync();
try
{
    await responseStream.ReadAsync(buffer);
}
catch (HttpResponseReadException ex)
{
    switch (ex.HttpRequestError)
    {
        // ....
    }
}

Risks

Any change to the error categorization is a breaking change, meaning that if we introduce new error codes for some users, we would break others.

Mobile handlers would need to implement their error mapping separately, meaning that we would lack platform consistency until that point. Whenever it's impossible to detect some error categories on those platforms, they would need to report a more generic error code eg. InvalidResponse instead of ResponseEnded. This is a challenge for users who need to include mobile targets in their cross-platform HttpClient code.

Since requests and connections are decoupled, connection-establishment errors (NameResolutionError, ConnectionError and SecureConnectionError) will be only reported if the originating request is still in the queue, otherwise we will swallow them. Observing these error codes is not a reliable way to observe ALL connection-level errors happening under the hood. Some users may find this surprising.

Alternative Designs

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation HttpRequestException is thrown under various conditions but it does not provide an error code to differentiate those conditions. Currently, it provides 'string Message' property inherited from Exception type and it can contain various error messages: - The HTTP response headers length exceeded the set limit - Cannot write more bytes to the buffer - The response ended prematurely - Received chunk header length could not be parsed - Received an invalid status - Received an invalid header line - Received an invalid header name - The server returned an invalid or unrecognized response - etc. The above strings are defined in https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/src/Resources/Strings.resx If we want to pragmatically handle exceptions based on the cause, it would be better dealing with error codes rather than dealing with string values. For example, when we create a dashboard for different exceptions encountered, aggregating by error codes would make more sense that doing by string values. And we could categorize some errors into a group for a special handling. Many other exceptions like SocketException and Win32Exception have an error code field already. Would it make sense to provide a new property for HttpRequestException to hold error codes? ### API Proposal ```csharp namespace System.Collections.Generic; public class HttpRequestException : Exception { ..... public HttpRequestError HttpRequestErrorCode; ..... } ``` ### API Usage ```csharp try { await httpClient.SendAsync(...); } catch (HttpRequestException ex) { switch (ex.HttpRequestErrorCode) { case: HttpRequestError.InvalidStatus: // do something break; default: break; } } ``` ### Alternative Designs _No response_ ### Risks _No response_
Author: searchmon
Assignees: -
Labels: `api-suggestion`, `area-System.Net.Http`
Milestone: -
Symbai commented 2 years ago

HttpRequestException has StatusCode property which already gives you the http status alias error code. (edit: Since .NET 5)

ManickaP commented 2 years ago

I'd love to see a comparison with other HTTP stack to see if they provide error codes like this or how do they report similar errors. Disadvantage of this is that it somewhat puts our internal workings on a public API - the newly exposed error codes depend on our internal logic. So if we were to re-haul HTTP parsing for example, we'd still need to do it in way to provide the same error codes in same situations or introduce new ones and obsolete the old ones. Whether this is limiting or not, needs to be determine. We might attempt to choose the error codes in way that it is common for any HTTP logic - for that we should compare with other stacks. For example WinHTTP does expose something similar: https://learn.microsoft.com/en-us/windows/win32/winhttp/error-messages and it also has several obsoleted error codes.

searchmon commented 2 years ago

HttpRequestException.StatusCode is HttpStatusCode type and didn't help in my cases where its value is null.

Another point is that, though HttpRequestException can be a leaf exception, it often has IOException as an inner exception. , which also misses a proper error code. (attached an example)

image

ManickaP commented 1 year ago

Triage: we'll decide if/how to solve this in 8.0 timeframe and if we'll do something, it'll aim at 8.0 release.

antonfirsov commented 1 year ago

Related requests from the past: #15567, #21447, #30896

MihaZupan commented 1 year ago

Another case where a more strongly-typed mechanism would be an improvement is the use of Exception.Data to communicate error conditions as part of the new H2 WebSockets feature. https://github.com/dotnet/runtime/blob/96b849ecfc8c535eafd01e0ae82c8a54488aacfb/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs#L441 https://github.com/dotnet/runtime/blob/96b849ecfc8c535eafd01e0ae82c8a54488aacfb/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs#L1074

Consuming this is rather ugly in YARP.

baronfel commented 1 year ago

We hit this same need in the .NET SDK built-in container tooling. To provide more actionable user feedback with AWS' container registry, we tried to detect server-side connection shutdowns that signaled a missing resource. To detect that case we had to compare on the message string, which felt very foreign and made everyone wonder if this was the correct way to accomplish the goal.

So A+ idea to have a more structured representation - looking forward to having it available.

antonfirsov commented 1 year ago

@baronfel question for clarity: you need to distinguish the case when server sends an EOF from other "invalid server response cases" right?

Note that the linked PR will miss some of those cases by doing Message.Equals("The response ended prematurely.", ...) instead of doing Message.StartsWith("The response ended prematurely", ...):

https://github.com/dotnet/runtime/blob/c84d95d846b7cb4144904fde5f09855251c10a70/src/libraries/System.Net.Http/src/Resources/Strings.resx#L312-L320

If we would define an error category (=error code) for the EOF case, it would most likely include all 3 of the above cases, therefore your catch block would convert exceptions in cases it doesn't do today. Any concerns with this?

baronfel commented 1 year ago

At least the cases found by Norm were specifically EOF, though I'm not sure if that's an exhaustive set of cases we would want to catch or just a point-in-time response. Since this particular handler is an attempt to detect and give the user a way to workaround a specific error scenario, I think any of the other response type you mention would be ok for us to catch as well. Any sort of hard-disconnect error from this particular API would require the same user mitigation - go create the missing resource through some other means.

rgroenewoudt commented 1 year ago

Android and iOS have their own native HTTP handlers and would need separate implementations to support this.

antonfirsov commented 1 year ago

@grendello would you be interested and able to implement this contract in the native handlers at some point to increase compatibility? Do you see any pitfalls looking at the proposed API?

Related: https://github.com/xamarin/xamarin-android/issues/5761

grendello commented 1 year ago

@antonfirsov I can implement that for Xamarin.Android, absolutely. Regarding the API I have a handful of immediate thoughts:

  1. It would be good if HttpRequestError members had numeric values of their corresponding HTTP codes
  2. HttpRequestError should have a member (with a very high value assigned, just to be safe) named, say, UnmappedHttpError (or UnknownHttpError)
  3. HttpRequestException should have a property returning the numeric value of the error (e.g. HttpErrorCode)

3. would allow filtering on e.g. ranges of error codes, and 2. could serve as a backward compatible fallback (e.g. when a client doesn't handle a given error code it is required to use UnmappedHttpError/UnknownHttpError)

antonfirsov commented 1 year ago

I can implement that for Xamarin.Android

Do you know who could provide some feedback about the iOS side?

It would be good if HttpRequestError members had numeric values of their corresponding HTTP codes

HttpRequestError cases and HTTP status codes are two different things. To observe the status code you can refer to HttpResponseMessage.StatusCode when using methods which do not throw in case of non-success status codes (eg. HttpClient.SendAsync(requestMessage) ) or HttpRequestException.StatusCode on methods that do throw (eg. GetStringAsync). HttpRequestError reports lower-level communication errors which some users may want to react to (eg. TLS failure, DNS resolution failure, server sent a premature EOF etc.).

UnmappedHttpError (or UnknownHttpError)

Instead of defining a "generic" error category, I prefer to make the excpetion property HttpRequestError? HttpRequestError { get; } optional.

HttpRequestException should have a property returning the numeric value of the error (e.g. HttpErrorCode)

You can always convert the enum to an integer, but we may consider this as a convenience feature.

grendello commented 1 year ago

@antonfirsov @rolfbjarne would probably be the person to talk to with regards to iOS (or he would be able to name the person who'd be able to work on it)

stephentoub commented 1 year ago

API Proposal

@antonfirsov, thanks for updating the proposal. Overall it seems reasonable, but some questions / comments:

  1. HttpRequestException.HttpRequestError shouldn't be nullable; that's very non-standard. Instead there should be a value in the enum that indicates Unknown, ideally with a value 0.
  2. Similarly the new ctors that accept the enum should just take an enum value rather than a nullable enum value. The ctors that don't take an enum value would just use Unknown.
  3. What error code is used when the response successfully returns a non-success status code?
  4. Why is HttpProtocolException needed? I understand the desire for HttpResponseReadException, since we want to continue throwing an IOException from streams, but why would we need yet another exception type derived from it?
antonfirsov commented 1 year ago
  1. HttpRequestException.HttpRequestError shouldn't be nullable; that's very non-standard. Instead there should be a value in the enum that indicates Unknown, ideally with a value 0.
  2. Similarly the new ctors that accept the enum should just take an enum value rather than a nullable enum value. The ctors that don't take an enum value would just use Unknown.

@stephentoub my reasoning was that this makes it very clear that the error value is optional and can be missing in many cases. But since it goes against our standards I will update the proposal to include a value of Undefined = 0.

  1. What error code is used when the response successfully returns a non-success status code?

The proposed API is only exposing errors on exceptions, and not on HttpResponseMessage. HttpRequestException is only thrown in Get* overloads that call response.EnsureSuccessStatusCode() internally. I didn't propose an error code for this case, since it will make the existing IsSuccessStatusCode property redundant. However, now I think it would help usability, so I will update the proposal to include one.

  1. Why is HttpProtocolException needed? I understand the desire for HttpResponseReadException, since we want to continue throwing an IOException from streams, but why would we need yet another exception type derived from it?

Sorry, I didn't make it clear that HttpProtocolException is an existing type, used to report HTTP/2 and HTTP/3 protocol errors. The proposal is changing its' base type to HttpResponseReadException.

bartonjs commented 1 year ago

Video

public class HttpRequestException : Exception
{
    public HttpRequestError? HttpRequestError { get; }

    public HttpRequestException(string? message, Exception? inner = null, HttpStatusCode? statusCode = null, HttpRequestError? httpRequestError = null) {}
}

// IOException subtype to throw from response read streams
public class HttpIOException : IOException  // ALTERNATIVE name: HttpResponseReadException
{
    public HttpRequestError HttpRequestError { get; }
    public HttpIOException(HttpRequestError httpRequestError, string? message = null, Exception? innerException = null) {}
} 

// The EXISTING HttpProtocolException should be the subtype of this new exception type
public class HttpProtocolException : HttpIOException
{
}

// Defines disjoint error categories with high granularity.
public enum HttpRequestError
{
    Unknown = 0,                          // Uncategorized/generic error

    NameResolutionError,                    // DNS request failed
    ConnectionError,                        // Transport-level error during connection
    TransportError,                         // Transport-level error after connection
    SecureConnectionError,                  // SSL/TLS error
    HttpProtocolError,                      // HTTP 2.0/3.0 protocol error occurred
    ExtendedConnectNotSupported,             // Extended CONNECT for WebSockets over HTTP/2 is not supported.
                                            // (SETTINGS_ENABLE_CONNECT_PROTOCOL has not been sent).
    VersionNegotiationError,                // Cannot negotiate the HTTP Version requested
    UserAuthenticationError,                // Authentication failed with the provided credentials
    ProxyTunnelError,

    InvalidResponse,                        // General error in response/malformed response

    // OPTIONAL TO NOT INCLUDE: The one's below are specific cases of errors with the response.
    // We may consider merging them into "InvalidResponse" and "InvalidResponseHeader"
    ResponseEnded,                          // Received EOF
    // These depend on SocketsHttpHandler configuration
    ConfigurationLimitExceeded,             // Response Content size exceeded MaxResponseContentBufferSize or
                                            // Response Header length exceeded MaxResponseHeadersLength or
                                            // any future limits are exceeded
}