runtime: StatusCode in HttpRequestException

i cam here just to comment that this seems like a fundamental hole in the API. it means that if we want to throw an exception indicating http status failure, we have to bake our own exception type. it seems crazy to me that such a fundamental thing - throwing an exception in an exceptional case shouldn’t include the reason for the exception. if you look around the web for this you’ll see people all over the place suggesting PARSING THE ERROR MESSAGE to deduce the status code! which is a terrible idea. but in the absence of a decent API, that’s what we’re left with.

The proposed API doesn’t work well in majority of the scenarios, and adding a new api for usability purposed for a particular narrow case doesn’t seem worth the cost. Closing.

For your scenario you could try catch the httprequestexception, and parse the string message for a statuscode and throw a different custom exception from your app.

Whilst I agree that the current API is deficient, let me suggest an alternative.

The real problem here is the EnsureSuccessStatusCode method. Through which we’re telling the framework that we want to rely on exceptions as a mechanism for control flow. Which, we should avoid, right?

Getting a 4xx or 5xx from a remote server is not exceptional - it’s a valid response. Being unabled to reach the remote server is however, an exception.

Consider also, that adding StatusCode to the exception would just be the start. In the case of a 400 we’d need to know about the full request - this includes the things we sent that weren’t explicit, like Content-Type header.

So that leaves us with parsing the response and avoiding lots of boilerplate code. Here, I would consider using a proxy for sending the request and intercepting the response. And if I really needed exceptions, I would throw here, where I had full control.

My request would be to deprecate EnsureSuccessStatusCode - it’s just a crutch at best.

For your scenario you could try catch the httprequestexception, and parse the string message for a statuscode and throw a different custom exception from your app.

This sounds like a joke from Redmond’s employee. Why the heck I should parse the string message in the 21st century if user input validation has failed with 400? How do I find that it was really an error due to user input or not internal 500 or conflict 409? I should parse string message for that? Unbelievable.

HttpRequestException is only thrown when an HTTP response can’t be obtained from the server. I.e. the server is down or other errors. So, there is no HTTP status code received at all since there is no HTTP response ever sent on the wire.

This is unlike the older HttpWebRequest API which will throw WebException for non-successful status code returns (i.e. not 2xx). That API has a WebResponse in the WebException if the exception is being caused due to non 2xx status code. For other errors, there is no WebResponse in the WebException.

So, I’m not sure the goal of your request to add a StatusCode to the HttpRequestException. Can you please clarify?

@davidsh HttpRequestException is thrown by the EnsureSuccessStatusCode method in the HttpResponseMessage class when IsSuccessStatusCode is false.

https://github.com/dotnet/corefx/blob/6b5ef121ebea45b14f489a177e2e3f27fce86781/src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs#L148-L165

I just did a search for HttpRequestException in our System.Net.Http codebase, and it is infact in a lot of cases where we use this exception without a statuscode like @davidsh remarks.

The internal implementation requirements of a component should not be the thing that dictates the nature of the external interface that the component presents to its users. It’s called designing from the outside in and is a pretty fundamental aspect of OOP. The fact that in this very thread the owners of this codebase are suggesting parsing the error message string and throwing a custom exception suggests someone has lost sight of the basics.

I hope that doesn’t come across as unduely derogatory, I don’t mean it to be. Just some frank honest criticism.

I find it amazing this was closed and dismissed without any workaround proposed in the framework. Having the status code is absolutely essential in some scenarios and now we’ll need to resort to custom code to add that information into it.

For your scenario you could try catch the httprequestexception, and parse the string message for a statuscode and throw a different custom exception from your app.

Are you aware that Exceptions are localized? Do you suggest to have a separate parser for every language?

HttpRequestException is only thrown when an HTTP response can’t be obtained from the server. I.e. the server is down or other errors. So, there is no HTTP status code received at all since there is no HTTP response ever sent on the wire.

A simple test demonstrates that this is not true. I have one web service IActionResult endpoint that returns UnauthorizedResult(). If I access this endpoint using HttpClient and call EnsureSuccessStatusCode it throws an HttpRequestException.

However, I don’t quite see the value in adding a StatusCode property to the HttpRequestException. In most cases, this property will be null.

This seems questionable to me. I don’t see any reason why HttpClient calls would be more likely to fail due to a connectivity issue than the server returning an error code. In an architecture where multiple servers call each other a transport error in a call to one server is almost certain to result in error codes being returned by multiple upsteam calls in progress.

It doesn’t make sense to add a StatusCode on this exception, because it is not true that every httprequestexception has a statuscode, it is not a property on the behavior of the type.

Not every exception has an InnerException, a HelpLink or Data dictionary values either, but that does not mean those properties are not useful elements of the Exception class interface.

Looking at the problem from the outside in, consumers of HttpClient would like to be able to handle cases where a request failed (i.e. server could not be reached or StatusCode >= 400) without having to clutter every HttpClient call location with ‘if’ statements and manual throws of custom exception types.

That seems perfectly reasonable and quite simple to achieve and the push back here strikes me as weakly substantiated and a little defensive.

If adding a StatusCode integer field is really off the cards (for whatever ideological reason), then a “less polluting” way to add it would be to use the already built-in exception Data dictionary. It was more or less made for these scenarios.

HttpResponseMessage.EnsureSuccessStatusCode() could add the integer status code to the Exception.Data dictionary with the key “StatusCode”. Then you could easily pull this back out with an extension method on HttpRequestException.

This is a one or two line change to HttpResponseMessage.EnsureSuccessStatusCode().

https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs

Current implementation:

public HttpResponseMessage EnsureSuccessStatusCode()
{
    if (!IsSuccessStatusCode)
    {
        throw new HttpRequestException(SR.Format(
        System.Globalization.CultureInfo.InvariantCulture,
        SR.net_http_message_not_success_statuscode,
        (int)_statusCode,
        ReasonPhrase));
    }

        return this;
}

Updated implementation:

public HttpResponseMessage EnsureSuccessStatusCode()
{
    if (!IsSuccessStatusCode)
    {
        var exception = new HttpRequestException(SR.Format(
        System.Globalization.CultureInfo.InvariantCulture,
        SR.net_http_message_not_success_statuscode,
        (int)_statusCode,
        ReasonPhrase));

        exception.Data["StatusCode"] = _statusCode;

        throw exception;
    }

    return this;
}

The extension method could look like:

public static HttpStatusCode? GetHttpStatusCode(this HttpRequestException httpRequestException) {
    if(httpRequestException.Data["StatusCode"] is HttpStatusCode statusCode) {
        return statusCode;
    }
    else {
        return null;
    }
}

You would probably want to move the “StatusCode” magic string into an internal static readonly string StatusCodeKey variable somewhere too.

In this way, the API surface of HttpRequestException doesn’t change, and the behavioral change in HttpResponseMessage.EnsureSuccessStatusCode() is unlikely to break anything. If you don’t use HttpResponseMessage.EnsureSuccessStatusCode(), you can simply not import the extension method’s namespace.

EDIT

Here’s a present-day workaround for adding the status code in an extension method:

public static class HttpResponseMessageExtensions
{
    public static HttpResponseMessage EnsureSuccessStatusCodeWithStatus(this HttpResponseMessage httpResponseMessage)
    {
        try
        {
            return httpResponseMessage.EnsureSuccessStatusCode();
        }
        catch (HttpRequestException ex)
        {
            ex.Data["StatusCode"] = httpResponseMessage.StatusCode;
            throw;
        }
    }
}

We actually implemented this in PR #32455 for .NET 5.

I have to agree with an earlier statement that it seems the real issue is the usefulness of EnsureSuccessStatusCode at all.

When you consider handling this in other ways it makes more sense. Throwing an exception when anything other than 2xx has happened?? The method itself is promoting using exceptions as logic flow, which we all know shouldn’t be done. This plus the fact that other status types, which will be given back, are perfectly fine responses. The Message has completed, you received a response back. Handle it!

If an exception is thrown because a response was never received then that is an exceptional circumstance and needs to be dealt with.

Seems to me that if this method didn’t exist at all there would actually be less problems.

Exposes a “ResponseMessage” (HttpResponseMessage) property instead of a “StatusCode” (int) property.

Thanks for thinking more about this.

However, adding an HttpResponseMessage isn’t something we would want to do. It would keep TCP connections open because the HttpResponseMessage.Content represents the HTTP response stream. That means that we have to change the pattern of handling HttpRequestException objects and explicitly dispose the inner HttpResponseMessage. Otherwise we end up with connections staying open

Usually, with objects have nested objects that are IDisposable, it would mean that HttpRequestException would be have to be IDisposable as well.

Definitely agree that if EnsureSuccessStatusCode throws an HttpRequestException, then at a minimum StatusCode would be a logical optional field to have (the value could even be stored in the HResult field!). However it is questionable whether receiving a valid HTTP response with a non 2xx status code is in fact an HttpRequestException - after all, the exception wasn’t with the HttpRequest object itself, it’s done its job - the full message has been successfully constructed and transmitted to the server, but what we get back happens not to be what we were hoping for. In fact, it might be not a valid HTTP response at all if something’s wrong with the backend server (what exception is thrown in this case btw?). Either way, if it isa valid HTTP response with a status code, an HttpRequestException on its own is not enough - I need to know whether it was a 4xx error or a 5xx error (because the former implies the caller did something wrong, and the latter implies the server did something wrong, or its a transient error where I can retry). But in fact I need more than just the status code - in many cases we almost certainly want the actual response body too (many APIs return, e.g. status code 400 but the accompanying JSON body gives the application-level error code/details). So I’d actually much rather that EnsureSuccessStatusCode threw an exception that referenced the full HttpResponse, much like the old WebException) I’d actually say in its current implementation EnsureSuccessStatusCode should never be used in production code.

HttpException is used for more than one thing (both connection errors, and protocol errors; problems on the transport layer and the application layer) which leads to an exception that is suited for the application layer, and introducing a status code would be irrelevant when the exception originated from a problem on the the transport layer.

This ought to be two different exceptions, SocketException and HttpException. With the HttpException extended with application layer information such as status code, or perhaps the entire HttpResponse.

To quote @davidsh

HttpRequestException is only thrown when an HTTP response can’t be obtained from the server. I.e. the server is down or other errors.

This ought to throw a SocketException instead of a HttpException.

This would allow code like:

try
{
    DoSomeHttp();
}
catch (SocketException e)
{
    Log("Failed to establish connection.");
}
catch (HttpRequestException e)
{
    Log($"Request failed with status code: {e.StatusCode}.");
}

Perhaps you could add a HttpRequestStatusException which inherits from HttpRequestStatusException and add this StatusCode property ?

Beside the fact that this exception is also used in cases when a server response is not available, the WebException was more useful with the full HttpResponse and the status code.