StatusCode in HttpRequestException

[vanillajonathan]

When the EnsureSuccessStatusCode method in the HttpResponseMessage throws a HttpRequestException exception it is not possible to determine the status code.

Add a new property named StatusCode to the HttpRequestException class which gets set by the EnsureSuccessStatusCode method of the HttpResponseMessage class.

Rationale and Usage

Allows you to determine the status code which triggered the exception.

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

private void DoSomeHttp()
{
    // ...
    message.EnsureSuccessStatusCode();
}

Proposed API

public class HttpRequestException : Exception
{
    // ...
    public HttpRequestException(string message, StatusCode statusCode);
    public HttpRequestException(string message, Exception inner, StatusCode statusCode);
    // ...
    public StatusCode StatusCode { get; }
    // ...
}

Open Questions

Should the property have a setter?
Should the exception have a constructor that sets the property?
Should the property be nullable?

[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. 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]

cc: @stephentoub

[vanillajonathan]

@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

[davidsh]

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

Yes, that is true. However, I don't quite see the value in adding a StatusCode property to the HttpRequestException. In most cases, this property will be null. In fact, it would have to be a nullable property since 0 can't be used since it could (in theory) be a status code.

What is the scenario where you need this property on the HttpRequestException object?

[vanillajonathan]

I've only come across HttpRequestException from EnsureSuccessStatusCode when dealing with HttpClient so I didn't know that it would be null in most cases, if so then perhaps they ought to be throwing different exceptions.

In my scenario I call a method that use HttpClient and when an exception is thrown from that method then I can catch it and if I had the status code available in the exception then I could log it, display something relevant message in the user interface or handle it in meaningful way such as offering authentication options.

[Priya91]

@vanillajonathan Can you update the original issue with api-proposal?

[vanillajonathan]

@Priya91 Done. :)

[Priya91]

The property should just be a getter, otherwise the code receiving the exception can change the status code, but the other values on the exception will be invalid.

You should add a contructor that takes in status code for the exception, and expose only getter on the property..

[vanillajonathan]

@Priya91 Okay I edited the post and declared the StatusCode property as as setter-only without a getter.
I also added constructors. The class have 3 constructors. Not sure how many needs to be added, maybe 3?

[Priya91]

@vanillajonathan 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. It is only in the case where you pointed out in code we have a statuscode, and it is given as part of the exception message. 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.

[Priya91]

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.

[Priya91]

In most cases, this property will be null. In fact, it would have to be a nullable property since 0 can't be used since it could (in theory) be a status code.

It could be kept as -1

[davidsh]

It could be kept as -1

Sure. But adding this property doesn't seem useful when the majority of time, it will be -1.

[Priya91]

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.

[Spongman]

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.

[stewart-ritchie-ps]

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.

[Neutrino-Sunset]

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.

[Neutrino-Sunset]

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.

[bo4arov]

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.

[wizofaus]

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.

[tbolon]

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.

[vanillajonathan]

Proposed API (revised)

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

public class HttpRequestException : Exception
{
    // ...
    public HttpRequestException(string message, HttpResponseMessage message);
    public HttpRequestException(string message, Exception inner, HttpResponseMessage message);
    // ...
    public HttpResponseMessage? ResponseMessage { get; }
    // ...
}

[davidsh]

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.

[wizofaus]

That's a fair point - so mayb there'd need to be a way of specifying "throw an exception where the full response body needs to be available by the handler", which would trigger reading the remaining response into a memory-backed stream, then close the connection.
Slightly tricky if there's a further error reading that remaining response of course! Actually in general with HttpClient, if there IS a network exception before the full response has been read, is there a way of retrieving whatever has been read up to that point? Or is reading the response data from the socket always delayed until the application tries to read from HttpResponseMessage.Content?

[vanillajonathan]

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}.");
}

[crozone]

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;
        }
    }
}

[wizofaus]

It's definitely better than the current version - there are a number of web service type calls out there where the only important thing is the status code, with no need to pull any extended error information from the response body.

[crozone]

You could optionally read the string result out of the response and into another key on the exception Data dictionary. However, it gets a little messy. For example, what happens if an error occurs during the read? Or what about if you want to want to use async? It's probably best to just create your own extension method or do the data handling manually if this is the use case. The status code is basically free.

[julealgon]

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.

[Predictor]

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?

[dpuckett]

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.

[vanillajonathan]

@dpuckett Then what would be a suitable way to write a function that performs a request and deserializes its JSON payload into an object, which seems like to be a very common scenario. Or is that doing two things (fetching AND deserializing) and hence a violation of the single-responsibility principle? Maybe just return null if the request was not successful?

Like you want to fetch a user from a HTTP endpoint.

private async Task<User> FetchUserAsync(string username)
{
    var response = await _client.GetAsync($"/api/users/{username}");
    if (!response.IsSuccessStatusCode)
    {
        return null;
    }
    var user = JsonSerializer.Deserialize<User>(response.Content);

    return user;
}

[dpuckett]

So a rough excerpt from a HttpClient Wrapper I have as an example.

public async Task PostAsync<T>(T item, string url)
{            
     var json = JsonConvert.SerializeObject(item);
     var content = new StringContent(json, Encoding.UTF8, "application/json");
     
     HttpResponseMessage response = await Client.PostAsync(new Uri(url, UriKind.Relative), content);
     if (response.IsSuccessStatusCode)
     {
         return;
     }
           
     var ex = new HttpRequestException(response.ReasonPhrase);
     if(response != null)
     {
         ex.Data.Add("StatusCode", response.StatusCode);
     } 
     throw ex;
}

So rather than using the EnsureSuccessStatusCode which automagically throws a rather useless Exception without the status code, I am still checking for success, and if not throwing my own exception and adding the StatusCode as a Data entry on it.

This way in my Service Layer/Application Layer I can wrap the method call in a try/catch and then handle appropriately if the request fails.

Sure it would be nice if EnsureSuccessStatusCode did this for us, but the fact remains that if there's no response at all for the method to run, it's a bit useless. This way we handle both cases.

[davidsh]

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

[davidsh]

Duplicate of #911