Runtime: Proposal: HttpRequestException w/ Status Code

[yaakov-h]

Latest Proposal

HttpRequestException.StatusCode property

Rationale and Usage

A new nullable HttpRequestException.StatusCode property exposes a response's status code if it has been already received, otherwise it's set to null. This will enable a direct access to the status code on exceptions thrown from HttpClient's simple GET methods and HttpResponseMessage.EnsureSuccessStatusCode(). New HttpRequestException constructors taking a status code will initialize that property.

```C#
try
{
string response = httpClient.GetStringAsync(requestUri);
...
}
catch(HttpRequestException e)
{
if (e.StatusCode == HttpStatusCode.MovedPermanently)
{
...
}
}

### Proposed API
```diff
public class HttpRequestException : Exception
{
        ...
+       public HttpStatusCode? StatusCode { get; }

        public HttpRequestException(string message)
        {...}

+       public HttpRequestException(string message, HttpStatusCode? statusCode)
+       {...}

        public HttpRequestException(string message, Exception inner)
        {...}

+       public HttpRequestException(string message, Exception inner, HttpStatusCode? statusCode)
+       {...}
}

---

Original Proposal

Proposed solution to dotnet/corefx#9227, dotnet/corefx#24253 and any related issues:

Linking in https://github.com/SteamRE/SteamKit/issues/517 as well.

I've had the following issue in various codebases:

1. Some HTTP-based code expects a 200-series response.
2. The HTTP-based code uses HttpResponseMessage.EnsureSuccessStatusCode()
3. The calling code catches the exception and wants to do something based on the status code.
4. The calling code does not have access to the original HTTP response status code, or any headers, etc.

In the old .NET HTTP APIs, this was straightforward because WebException includes the status code and the response object, where available.

Adding StatusCode to HttpRequestException has been rejected in the above-linked issues due to it being of little use in most cases.

Would it be possible instead to create a subclass of HttpRequestException, and have EnsureSuccessStatusCode() throw that instead?

Something resembling the following ought to do the trick:

```c#
public class HttpResponseException : HttpRequestException
{
public HttpResponseException()
{
...
}

public HttpResponseException(string message, Exception innerException)
{
    ...
}

public HttpResponseException(string message, int statusCode)
{
    ...
}

public HttpResponseException(string message, int statusCode, Exception innerException)
{
    ...
}

public int StatusCode { get; }

}
```

This would achieve the following:

1. HttpRequestException does not have a majority-useless field.
2. Code that catches HttpRequestException would continue to function as-is.
3. Networking errors, unreachable servers etc. will continue to throw HttpRequestException.
4. New code can opt in to catching HttpResponseException, and will then have access to the original HTTP Status Code.

Optionally, for consideration, the exception could also include the whole HttpResponseMessage object in line with good old WebException.

[davidsh]

Thank you for opening this issue. But I thought we discussed this in https://github.com/dotnet/corefx/issues/24253 and decided that having a different exception was not a good pattern.

For code that wants to use a similar pattern of HttpWebRequest where non 2xx status codes can have exceptions returned, we encourage the use the HttpClient.EnsureSuccessStatusCode() method:

https://stackoverflow.com/questions/21097730/usage-of-ensuresuccessstatuscode-and-handling-of-httprequestexception-it-throws

cc: @karelz @Priya91

[yaakov-h]

I don’t see any discussion of a new exception type in that issue.

I can understand encouraging EnsureSuccessStatusCode, however the exception thrown is missing a couple key pieces of data which made WebException in older HttpWebRequest/WebClient-based code so useful / usable.

[Priya91]

@yaakov-h This issue is a dupe of dotnet/corefx#24253 , how is this issue different from that one?

[yaakov-h]

dotnet/corefx#24253 proposed adding rarely-used fields to an existing exception type.

This issue proposes adding a new exception type for this specific purpose.

[davidsh]

Adding a new exception type would be a large change and would break existing API behavior. As I mentioned above, if you need an exception to be thrown for a valid, but unsuccessful, HTTP status code, we recommend using the HttpClient.EnsureSuccessStatusCode() method.

[karelz]

Let's close it as duplicate of dotnet/corefx#24253

Both are trying to solve the same problem - each of them different way, but the same underlying problem "ability to access HttpStatusCode upon error". It makes sense to track problems and find best solution for them, rather than track competing solutions.

[EDIT] Reopened later as the alleged dupe is closed.

[yaakov-h]

Can we re-open dotnet/corefx#24253 then?

@davidsh: How would it change existing API behaviour?

Furthermore, this entire issue and the linked ones are trying to solve a deficiency in HttpResponseMessage.EnsureSuccessStatusCode() (there is no such method on HttpClient), so telling me to use that doesn't solve the problem, it causes it.

[davidsh]

there is no such method on HttpClient

Sorry meant to say HttpResponseMessage.EnsureSuccessStatusCode and not HttpClient.

[karelz]

Ooops, I didn't notice dotnet/corefx#24253 is closed. Reopening - we may close/reject it again, but from different reason than being a dupe.

[svick]

@davidsh

Adding a new exception type would be a large change and would break existing API behavior.

How would that be a breaking change? The breaking change rules explicitly mention that throwing a more derived exception is allowed.

[anewcomer]

@davidsh @Priya91

I think all that people are asking for is that the status code that is embedded in the HttpRequestException message in EnsureSuccessStatusCode be made available to catchers of the thrown exception without having to parse the message string.

Maybe that means extending HttpRequestException with a nullable int or nullable HttpStatusCode property. Maybe that means a whole other derived exception type. Maybe that means using the Exception.Data property.

There's a few options, and I don't think folks will be too picky. All they really want is a way to get the response status code without parsing the exception message.

Personally, I'd prefer a StatusCode property on the exception to make catch-when filtering nice and clean:
c# try { // make call } catch (HttpRequestException ex) when (e.StatusCode == 404) { // do something } catch (HttpRequestException ex) when (e.StatusCode == 503) { // do something }

[lorcanmooney]

I'd like to see this too - preferably as a nullable property on the existing exception type.

The GetStringAsync, GetByteArrayAsync and GetStreamAsync helper methods on HttpClient currently don't provide any way to determine why a request failed, and this proposal (whichever form it takes) would solve the common scenarios for me.

[danmosemsft]

@davidsh to see comment above since it seems that several folks are interested in this. I agree that a derived exception type is rarely considered breaking, but I don't have context on the issue itself.

[davidsh]

@davidsh to see comment above since it seems that several folks are interested in this. I agree that a derived exception type is rarely considered breaking, but I don't have context on the issue itself.

I'm working on an API proposal that will extend HttpRequestException to provide for this StatusCode property as well as another property to handle storing platform-independent error information (similar to WebException.Status property). See dotnet/corefx#19185

[Spongman]

i cannot believe there is such resistance to exposing the http status code to the calling code. the old WebClient api provided a mechanism for retrieving this. it's amazing to me that the recommended way to find out what caused the exception it to parse the error string - this is insane.

[ichensky]

When creating web crawlers, very often needed to detect what code server returned: HTTP Code 500 or HTTP Code 502 and so on.
It would be very good, if we add HttpStatusCode to the HttpRequestException .

[yaakov-h]

@karelz @davidsh @danmosemsft what needs to happen in order to solve this? Anything internal, roadmap, schedule, PR...?

[davidsh]

the old WebClient api provided a mechanism for retrieving this.

The prior Web APIs like HttpWebRequest would always throw a WebException for any non-successful (non 2xx) status code. And it would throw WebException as well for other network errors. So, it was important and useful that the WebException had HTTP response information (such as StatusCode) in the exception. That was because there may or may not be an actual HTTP protocol response even though the WebException is thrown.

In today's HttpClient API, HttpRequestException is not thrown by the HttpClient APIs except for networking errors (like can't connect etc). All HTTP responses containing any StatusCode will be returned. This includes success status codes like 200 or non-success status codes like 500.

The discussion is this issue derives from the use of the EnsureSuccessStatusCode API which can be called subsequently after the regular HttpClient APIs. Calling that API will thrown an HttpRequestException if the StatusCode contained in the HttpResponseMessage is a non-success status code.

However, the StatusCode can still be obtained easily from the original HttpResponseMessage returned by the HttpClient APIs prior to any call of EnsureSuccessStatusCode. So, the use pattern of HttpClient APIs is very different from HttpWebRequest APIs.

This is why it doesn't seem as important to add a StatusCode field to the HttpRequestException because in the majority of cases, there won't be any StatusCode at all (it would have to be a nullable field) since the HttpRequestException thrown by HttpClient APIs typically is due to networking errors where no HTTP response message is ever returned.

So, the argument here is whether or not it is a good API design to add a field to the HttpRequestException object where most of the time it will be null.

[davidsh]

@stephentoub @geoffkizer

[karelz]

@yaakov-h it is on our short list of top backlog asks for .NET Core 3.0 (the milestone reflects that). It has a decent chance to be done in 3.0 (no guarantee though).

[karelz]

Given @davidsh's summary and insights, I feel more that it is weird to mimic this old style HttpWebRequest-way of surfacing StatusCode to callers from HttpClient via exceptions. I am actually leaning more towards closing it as By Design / Won't Fix.

If you are interested in this feature, DESPITE the fact that HttpClient provides a reasonable way to get to StatusCode (even without exceptions being thrown), can you please:

1. Upvote top post (currently there are 0 upvotes)
2. Clarify your use case - why is using (slower) exception handling better/easier for your code? Is it just because of migration from HttpWebRequest, or is more involved?

Thanks!

(closed by accident - the buttons are too close to each other :(, sorry)

[Spongman]

However, the StatusCode can still be obtained easily from the original HttpResponseMessage returned by the HttpClient APIs prior to any call of EnsureSuccessStatusCode.

i don't understand this. You're basically saying that since EnsureSuccessStatusCode doesn't return the failure code, it's useless and we should re-implement it ourselves?

And then you use that 'argument' (?) as the basis for rejecting this request?

I know this is just opinions here, but that reasoning seems specious at best.

[davidsh]

i don't understand this. You're basically saying that since EnsureSuccessStatusCode doesn't return the failure code, it's useless and we should re-implement it ourselves?

EnsureSuccessStatusCode is just a convenience method that checks the HttpResponseMessage and then throws an exception. But it doesn't have to be used. Many times, not throwing exceptions results in faster performance. That was an important consideration when HttpClient was designed. HttpWebRequest threw exceptions all the time even when a valid HTTP response was received.

```c#
public bool IsSuccessStatusCode
{
get { return ((int)_statusCode >= 200) && ((int)_statusCode <= 299);
}

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

return this;

}
`` So, the best-practice pattern we recommend when using HttpClient is not to useEnsureSucessStatusCodeat all. That API was added just to mimic HttpWebRequest behaviors. Using this API generates exceptions which slows down code. If you just need to check the status code of an HttpResponseMessage, it would be better to use the single 'if' check and then do your business logic accordingly. And you can do that with the publicIsSuccessStatusCode` method. This avoid any extra exceptions being thrown.

[yaakov-h]

This assumes that you're the direct consumer of the HttpResponseMessage and any details that it provides.

If you are building a library, for example, then:

• You don't need to expose the underlying HttpResponseMessage, probably shouldn't, and sometimes can't.
• For a good consumer API you would want the return object to follow the 99% use-case, and can use exceptions for rare failures. If you create a return object that encompasses all scenarios, many consumers will ignore the failure cases anyway.

In all cases, though:

• For rare cases of a bad response, such as a 500/503 when the services are down, or 504 gateway timeout, the performance hit from exceptions are often not a concern, particularly given that we're talking milliseconds or less, and a web request is often hundreds of milliseconds and varies based on network performance, hardware, the phase of the moon, etc.
• For a bad response, it is still often useful for consumers to handle the status code.
• A lot of codebases are still migrating from HttpWebRequest or WebClient and it is simpler to continue the behaviour of "success = ok, failure = throw" than re-architect applications, including breaking changes in libraries to build out an entirely new exception-less API design.
• I also don't buy the exception-performance argument considering that HttpRequestException is also thrown in cases where there is no valid HTTP response (DNS failure, etc.), or TaskCanceledException for timeouts. The system isn't entirely exception-less, so drawing a line at "once we have a parsed HTTP response, don't use exceptions because they are slow" seems odd and arbitrary.

[lorcanmooney]

In today's HttpClient API, HttpRequestException is not thrown by the HttpClient APIs except for networking errors (like can't connect etc).

However, the StatusCode can still be obtained easily from the original HttpResponseMessage returned by the HttpClient APIs prior to any call of EnsureSuccessStatusCode.

If you are interested in this feature, DESPITE the fact that HttpClient provides a reasonable way to get to StatusCode (even without exceptions being thrown)...

@davidsh @karelz This is so very frustrating. You keep saying things to this effect, but as I've already pointed out in this very thread, it's simply not the case. HttpClient already provides a bunch of exception-based APIs which throw when they encounter HTTP errors and, in those cases, the status code is _not_ available.

Clarify your use case - why is using (slower) exception handling better/easier for your code? Is it just because of migration from HttpWebRequest, or is more involved?

I don't understand the fascination with performance. For me, the use case is simplicity and readability. I'd say that over 90% of the time when I use HttpClient I want to make some simple GET requests. I have no desire to deal with request/response messages. The exception-based GetStringAsync (and friends) is ideal for writing succinct readable code, but the instant I need to special-case a HTTP error, I have to reinvent code that already exists in the framework.

The performance overhead of an exception being thrown simply doesn't enter into the equation.

[wasker]

Apparently, HttpRequestException is not only thrown by EnsureSuccessStatusCode. Below is an example of an exception occurring if AKV client is unable to connect to the KV at all (source of exception is HttpClient). This whole thing becomes real problem if one to implement retry policy for AKV client, which only has access to the exception, not other objects. How one tell apart HttpRequestException due to inability to connect (retriable) vs. HttpRequestException due to 401 or 403 (no point in retrying)?

Unhandled Exception: System.Net.Http.HttpRequestException: An error occurred while sending the request. ---> System.Net.WebException: The remote name could not be resolved: 'blah-blah.vault.azure.net'
   at System.Net.HttpWebRequest.EndGetResponse(IAsyncResult asyncResult)
   at System.Net.Http.HttpClientHandler.GetResponseCallback(IAsyncResult ar)
   --- End of inner exception stack trace ---
   at Microsoft.Rest.RetryDelegatingHandler.<SendAsync>d__15.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Net.Http.HttpClient.<FinishSendAsyncBuffered>d__58.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Azure.KeyVault.KeyVaultCredential.<ProcessHttpRequestAsync>d__13.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Azure.KeyVault.KeyVaultClient.<GetSecretsWithHttpMessagesAsync>d__66.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Azure.KeyVault.KeyVaultClientExtensions.<GetSecretsAsync>d__50.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Extensions.Configuration.AzureKeyVault.AzureKeyVaultConfigurationProvider.<LoadAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.Extensions.Configuration.AzureKeyVault.AzureKeyVaultConfigurationProvider.Load()
   at Microsoft.Extensions.Configuration.ConfigurationRoot..ctor(IList`1 providers)
   at Microsoft.Extensions.Configuration.ConfigurationBuilder.Build()
   at Microsoft.AspNetCore.Hosting.WebHostBuilder.BuildCommonServices(AggregateException& hostingStartupErrors)
   at Microsoft.AspNetCore.Hosting.WebHostBuilder.Build()
   at Program.Main(String[] args) in

[yaakov-h]

@karelz What needs to be done next to get the ball rolling on this? A formal API design? A PR with implementation? Thoughts and prayers?

[karelz]

It needs to get thought through from all angles - all concerns above addressed.
At this point a summary listing possibilities and their pros/cons would be likely next best step.

[anurse]

FYI, this is valuable for SignalR customers (see https://github.com/aspnet/AspNetCore/issues/17304) so we'd support getting StatusCode exposed on the exception somewhere (derived exception, nullable property, whatever 😉).

[scalablecory]

@anurse Can you go into how SignalR would make use of this?

We can already get a status code out of the HttpResponseMessage, so is it primarily for when we use the convenience APIs like HttpClient.GetStringAsync()?

Or is the intent that if some other e.g. protocol problem is encountered, but we did get a status code before that problem, we would still have that status code here? I can see this being somewhat useful for managing errors in non-idempotent requests.

[anurse]

@scalablecory It's not that SignalR uses it, it's that we call EnsureSuccessStatusCode and when it throws we have customers asking for the status code (linked issue above). We'd prefer it if the client threw an exception that had the status code. Putting our own exception type in is an option, but we'd prefer not to do that when it feels like this data could be exposed by corefx.

I just wanted to raise the fact that we have requests from this coming in at our end as well.

[scalablecory]

Thanks @anurse, that aligns with my thoughts.

[scalablecory]

Something like this might give a little more flexibility:

```c#
public class HttpRequestException
{
public HttpResponseMessage? Response { get; }
}

But I suspect that could present some usability issues with disposed responses. Need to look at implementation a bit.

Otherwise, something like this seems reasonable:

```c#
public class HttpRequestException
{
   public HttpStatusCode StatusCode { get; }
}

[davidsh]

public HttpResponseMessage? Response { get; }

Having an HttpResponseMessage in an exception means that it will keep connections open etc. So, then, do we add an Dispose() method to the HttpRequestException? It isn't IDisposable at all. And having a pattern where complex types are inside exceptions is very confusing.

So, I would suggest just adding the 'StatusCode'. And it needs to be a nullable type as well.

[scalablecory]

Having an HttpResponseMessage in an exception means that it will keep connections open etc. So, then, do we add an Dispose() method to the HttpRequestException? It isn't IDisposable at all. And having a pattern where complex types are inside exceptions is very confusing.

Yes, that was my concern as well.

[alnikola]

HttpRequestException.StatusCode property

Rationale and Usage

A new nullable HttpRequestException.StatusCode property exposes a response's status code if it has been already received, otherwise it's set to null. This will enable a direct access to the status code on exceptions thrown from HttpClient's simple GET methods and HttpResponseMessage.EnsureSuccessStatusCode(). New HttpRequestException constructors taking a status code will initialize that property.

```C#
try
{
string response = httpClient.GetStringAsync(requestUri);
...
}
catch(HttpRequestException e)
{
if (e.StatusCode == HttpStatusCode.MovedPermanently)
{
...
}
}

### Proposed API
```diff
public class HttpRequestException : Exception
{
        ...
+       public HttpStatusCode? StatusCode { get; }

        public HttpRequestException(string message)
        {...}

+       public HttpRequestException(string message, HttpStatusCode? statusCode)
+       {...}

        public HttpRequestException(string message, Exception inner)
        {...}

+       public HttpRequestException(string message, Exception inner, HttpStatusCode? statusCode)
+       {...}
}

@yaakov-h Do you agree with this proposal? If yes, I will update the description.

[yaakov-h]

@alnikola yep, that does seem to be the simplest way to proceed, I like it.

The only question I have on it - should StatusCode be an int?, or System.Net.HttpStatusCode??

Checking the value would typically require a bit of casting, since everything else under System.Net.Http seems to use HttpStatusCode, and I think the standard set of non-enum integer constants is part of ASP.NET Core, not the base .NET Core.

[alnikola]

Yes, you are right HttpStatusCode would be better. I will update the comment and description.

[terrajobst]

Video

• Looks good
• At the minimum we'll need to remove the nullable annotation fromt he first constructor, otherwise passing null as the second argument would be a source breaking change
• However, we concluded we can just remove it.

```C#
public class HttpRequestException : Exception
{
// Existing:
// public HttpRequestException(string message);
// public HttpRequestException(string message, Exception inner);

public HttpRequestException(string message, Exception inner, HttpStatusCode? statusCode);
public HttpStatusCode? StatusCode { get; }

}
```

[yaakov-h]

Thanks @terrajobst, @alnikola, and everyone else in getting it through API review.

Interesting discussion about re-using HResult, though I do agree the decision to _not_ use that.

What are the next steps? Can I just open a Pull Request with these changes + unit tests?

[alnikola]

@yaakov-h Yes, you can now open a PR.

[alnikola]

If you have some questions or need help with something I will be glad to assist.

Additionally, if you want to update the documentation by yourself as well, I can show how to do it. If no, I will do it by myself after the PR is completed.

[yaakov-h]

Thanks.

I'll have a look tonight, but my current thinking is:

1. Add property and constructor, as approved, to Exception object.
2. Add to reference assembly, if that's maintained separately?
3. Add tests that it's set when it should be and not when it's not.
4. Add tests that the property gets persistent when the exception is serialized. I assume that's still a thing after serialization was ported from .NET Framework?
5. Update callsites to include this when constructing the exception
6. Add tests for callsites to ensure that they do

And then for the docs:

1. Update docs for HttpRequestException to document the new property
2. Update docs for EnsureSuccessStatusCode to point developers at the new property?

[alnikola]

1. OK
2. Please check this guideline on updating ref assemblies
3. OK
4. Yes, you need to make sure it's serialized properly
5. OK
6. OK

Docs:

1. Please, don't forget to document the new constructor as well
2. Yes, it's worth mentioning that now developers have access to the status code

[yaakov-h]

Thanks. I've opened a draft pull request at https://github.com/dotnet/runtime/pull/32455.

If the exception isn't marked as serializable, what (if anything) should I do regarding serialization? There's no [Serializable] attribute and no pre-existing serialization constructor.

Also, what's with updating the docs? Are those in a separate repo? I couldn't find anything relevant under docs/.

[alnikola]

To update the docs, you need to go to HttRequestException page and click on "Edit" button at the top-right. It will bring you to dotnet/dotnet-api-docs repo where you can create a PR.

Serialization was already discussed on the relevant PR.

[yaakov-h]

oh gee, that XML looks quite a bit more complicated than I was expecting

[davidsh]

Closed by #32455