-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
StatusCode in HttpRequestException #23648
Comments
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 |
cc: @stephentoub |
@davidsh |
Yes, that is true. However, I don't quite see the value in adding a What is the scenario where you need this property on the HttpRequestException object? |
I've only come across In my scenario I call a method that use |
@vanillajonathan Can you update the original issue with api-proposal? |
@Priya91 Done. :) |
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.. |
@Priya91 Okay I edited the post and declared the |
@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. |
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. |
It could be kept as -1 |
Sure. But adding this property doesn't seem useful when the majority of time, it will be -1. |
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. |
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 underneath status code of this exception[3] is tricky to catch (see its shitty API and Microsoft's unwillingness to fix this mess here[1]) as it comes just inside the exception message (plus to add insult to injury, CloudFlare has invented their own[2] codes). [1] https://github.com/dotnet/corefx/issues/24253 [2] https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#Cloudflare [3] [ERROR] FATAL UNHANDLED EXCEPTION: System.AggregateException: One or more errors occurred. ---> System.Exception: Some problem when connecting to https://etc-parity.callisto.network ---> System.AggregateException: One or more errors occurred. ---> Nethereum.JsonRpc.Client.RpcClientUnknownException: Error occurred when trying to send rpc requests(s) ---> System.Net.Http.HttpRequestException: 522 (Origin Connection Time-out) at System.Net.Http.HttpResponseMessage.EnsureSuccessStatusCode () [0x0002a] in <9f3d2d95936b4d78aa542c9f32650f41>:0 at Nethereum.JsonRpc.Client.RpcClient+<SendAsync>d__17.MoveNext () [0x0012a] in <637b104e55b941dcb1efbb91e63ee797>:0 --- End of inner exception stack trace --- at Nethereum.JsonRpc.Client.RpcClient+<SendAsync>d__17.MoveNext () [0x00286] in <637b104e55b941dcb1efbb91e63ee797>:0 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () [0x0000c] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Threading.Tasks.Task task) [0x0003e] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification (System.Threading.Tasks.Task task) [0x00028] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd (System.Threading.Tasks.Task task) [0x00008] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1+ConfiguredTaskAwaiter[TResult].GetResult () [0x00000] in <e22c1963d07746cd9708456620d50e1a>:0 at Nethereum.JsonRpc.Client.RpcClient+<SendInnerRequestAync>d__12`1[T].MoveNext () [0x000a2] in <637b104e55b941dcb1efbb91e63ee797>:0 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () [0x0000c] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Threading.Tasks.Task task) [0x0003e] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification (System.Threading.Tasks.Task task) [0x00028] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd (System.Threading.Tasks.Task task) [0x00008] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1+ConfiguredTaskAwaiter[TResult].GetResult () [0x00000] in <e22c1963d07746cd9708456620d50e1a>:0 at Nethereum.JsonRpc.Client.ClientBase+<SendRequestAsync>d__4`1[T].MoveNext () [0x0014f] in <5b4585c1c99947d78244d74142f50fce>:0 --- End of inner exception stack trace --- at System.Threading.Tasks.Task.ThrowIfExceptional (System.Boolean includeTaskCanceledExceptions) [0x00011] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Threading.Tasks.Task.Wait (System.Int32 millisecondsTimeout, System.Threading.CancellationToken cancellationToken) [0x00043] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Threading.Tasks.Task.Wait (System.TimeSpan timeout) [0x00022] in <e22c1963d07746cd9708456620d50e1a>:0 at GWallet.Backend.Ether.Server.WaitOnTask[T,R] (Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] func, T arg) [0x0003d] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at [email protected] (Nethereum.Web3.Web3 web3, System.String publicAddress) [0x00013] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at Microsoft.FSharp.Core.OptimizedClosures+Invoke@3253[T2,TResult,T1].Invoke (T2 u) [0x00001] in <59964427904cf4daa745038327449659>:0 at GWallet.Backend.Ether.Server+GetUnconfirmedEtherBalance@133-2.Invoke (System.String arg10) [0x00001] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at Microsoft.FSharp.Core.FSharpFunc`2[T,TResult].InvokeFast[V] (Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] func, T arg1, TResult arg2) [0x0001f] in <59964427904cf4daa745038327449659>:0 at GWallet.Backend.Ether.Server+serverFuncs@101[T,R].Invoke (GWallet.Backend.Ether.Server+SomeWeb3 web3, T arg) [0x00002] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 --- End of inner exception stack trace --- at <StartupCode$GWallet-Backend>.$FaultTolerantParallelClient+asyncJobsToRunInParallelAsAsync@134-3[E,R].Invoke (System.Exception _arg1) [0x000a9] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at Microsoft.FSharp.Control.AsyncBuilderImpl+tryWithExnA@881[a].Invoke (System.Runtime.ExceptionServices.ExceptionDispatchInfo edi) [0x0000d] in <59964427904cf4daa745038327449659>:0 at Microsoft.FSharp.Control.AsyncBuilderImpl+callA@839[b,a].Invoke (Microsoft.FSharp.Control.AsyncParams`1[T] args) [0x00052] in <59964427904cf4daa745038327449659>:0 --- End of inner exception stack trace --- at System.Threading.Tasks.Task.ThrowIfExceptional (System.Boolean includeTaskCanceledExceptions) [0x00011] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Threading.Tasks.Task`1[TResult].GetResultCore (System.Boolean waitCompletionNotification) [0x0002b] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Threading.Tasks.Task`1[TResult].get_Result () [0x0000f] in <e22c1963d07746cd9708456620d50e1a>:0 at GWallet.Backend.FaultTolerantParallelClient`1[E].WhenSomeInternal[T,R] (System.Int32 numberOfResultsRequired, Microsoft.FSharp.Collections.FSharpList`1[T] tasks, Microsoft.FSharp.Collections.FSharpList`1[T] resultsSoFar, Microsoft.FSharp.Collections.FSharpList`1[T] failedFuncsSoFar) [0x0009a] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at GWallet.Backend.FaultTolerantParallelClient`1[E].WhenSome[T,R] (System.Int32 numberOfConsistentResultsRequired, System.Collections.Generic.IEnumerable`1[T] jobs, Microsoft.FSharp.Collections.FSharpList`1[T] resultsSoFar, Microsoft.FSharp.Collections.FSharpList`1[T] failedFuncsSoFar) [0x00012] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at GWallet.Backend.FaultTolerantParallelClient`1[E].QueryInternal[T,R] (T args, Microsoft.FSharp.Collections.FSharpList`1[T] funcs, Microsoft.FSharp.Collections.FSharpList`1[T] resultsSoFar, Microsoft.FSharp.Collections.FSharpList`1[T] failedFuncsSoFar, System.UInt16 retries) [0x001cf] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at <StartupCode$GWallet-Backend>.$FaultTolerantParallelClient+Query@165[R,E,T].Invoke (Microsoft.FSharp.Core.Unit unitVar) [0x00023] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at Microsoft.FSharp.Control.AsyncBuilderImpl+callA@839[b,a].Invoke (Microsoft.FSharp.Control.AsyncParams`1[T] args) [0x00052] in <59964427904cf4daa745038327449659>:0 --- End of stack trace from previous location where exception was thrown --- at GWallet.Backend.Infrastructure.Report[a] (System.Exception ex) [0x0000c] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at GWallet.Backend.Infrastructure.OnUnhandledException[a] (System.Object sender, System.UnhandledExceptionEventArgs args) [0x00007] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at [email protected] (System.Object sender, System.UnhandledExceptionEventArgs e) [0x00001] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 ---> (Inner Exception #0) System.Exception: Some problem when connecting to https://etc-parity.callisto.network ---> System.AggregateException: One or more errors occurred. ---> Nethereum.JsonRpc.Client.RpcClientUnknownException: Error occurred when trying to send rpc requests(s) ---> System.Net.Http.HttpRequestException: 522 (Origin Connection Time-out) at System.Net.Http.HttpResponseMessage.EnsureSuccessStatusCode () [0x0002a] in <9f3d2d95936b4d78aa542c9f32650f41>:0 at Nethereum.JsonRpc.Client.RpcClient+<SendAsync>d__17.MoveNext () [0x0012a] in <637b104e55b941dcb1efbb91e63ee797>:0 --- End of inner exception stack trace --- at Nethereum.JsonRpc.Client.RpcClient+<SendAsync>d__17.MoveNext () [0x00286] in <637b104e55b941dcb1efbb91e63ee797>:0 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () [0x0000c] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Threading.Tasks.Task task) [0x0003e] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification (System.Threading.Tasks.Task task) [0x00028] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd (System.Threading.Tasks.Task task) [0x00008] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1+ConfiguredTaskAwaiter[TResult].GetResult () [0x00000] in <e22c1963d07746cd9708456620d50e1a>:0 at Nethereum.JsonRpc.Client.RpcClient+<SendInnerRequestAync>d__12`1[T].MoveNext () [0x000a2] in <637b104e55b941dcb1efbb91e63ee797>:0 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () [0x0000c] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess (System.Threading.Tasks.Task task) [0x0003e] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification (System.Threading.Tasks.Task task) [0x00028] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd (System.Threading.Tasks.Task task) [0x00008] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1+ConfiguredTaskAwaiter[TResult].GetResult () [0x00000] in <e22c1963d07746cd9708456620d50e1a>:0 at Nethereum.JsonRpc.Client.ClientBase+<SendRequestAsync>d__4`1[T].MoveNext () [0x0014f] in <5b4585c1c99947d78244d74142f50fce>:0 --- End of inner exception stack trace --- at System.Threading.Tasks.Task.ThrowIfExceptional (System.Boolean includeTaskCanceledExceptions) [0x00011] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Threading.Tasks.Task.Wait (System.Int32 millisecondsTimeout, System.Threading.CancellationToken cancellationToken) [0x00043] in <e22c1963d07746cd9708456620d50e1a>:0 at System.Threading.Tasks.Task.Wait (System.TimeSpan timeout) [0x00022] in <e22c1963d07746cd9708456620d50e1a>:0 at GWallet.Backend.Ether.Server.WaitOnTask[T,R] (Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] func, T arg) [0x0003d] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at [email protected] (Nethereum.Web3.Web3 web3, System.String publicAddress) [0x00013] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at Microsoft.FSharp.Core.OptimizedClosures+Invoke@3253[T2,TResult,T1].Invoke (T2 u) [0x00001] in <59964427904cf4daa745038327449659>:0 at GWallet.Backend.Ether.Server+GetUnconfirmedEtherBalance@133-2.Invoke (System.String arg10) [0x00001] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at Microsoft.FSharp.Core.FSharpFunc`2[T,TResult].InvokeFast[V] (Microsoft.FSharp.Core.FSharpFunc`2[T,TResult] func, T arg1, TResult arg2) [0x0001f] in <59964427904cf4daa745038327449659>:0 at GWallet.Backend.Ether.Server+serverFuncs@101[T,R].Invoke (GWallet.Backend.Ether.Server+SomeWeb3 web3, T arg) [0x00002] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 --- End of inner exception stack trace --- at <StartupCode$GWallet-Backend>.$FaultTolerantParallelClient+asyncJobsToRunInParallelAsAsync@134-3[E,R].Invoke (System.Exception _arg1) [0x000a9] in <5ac5b2d75952d4f0a7450383d7b2c55a>:0 at Microsoft.FSharp.Control.AsyncBuilderImpl+tryWithExnA@881[a].Invoke (System.Runtime.ExceptionServices.ExceptionDispatchInfo edi) [0x0000d] in <59964427904cf4daa745038327449659>:0 at Microsoft.FSharp.Control.AsyncBuilderImpl+callA@839[b,a].Invoke (Microsoft.FSharp.Control.AsyncParams`1[T] args) [0x00052] in <59964427904cf4daa745038327449659>:0 <---
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. |
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.
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.
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. |
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. |
This sounds like a joke from Redmond's employee. |
Definitely agree that if |
Perhaps you could add a Beside the fact that this exception is also used in cases when a server response is not available, the |
Proposed API (revised)Exposes a "ResponseMessage" ( public class HttpRequestException : Exception
{
// ...
public HttpRequestException(string message, HttpResponseMessage message);
public HttpRequestException(string message, Exception inner, HttpResponseMessage message);
// ...
public HttpResponseMessage? ResponseMessage { get; }
// ...
} |
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. |
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. |
This ought to be two different exceptions, To quote @davidsh
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}.");
} |
If adding a
This is a one or two line change to Current implementation:
Updated implementation:
The extension method could look like:
You would probably want to move the "StatusCode" magic string into an In this way, the API surface of EDIT Here's a present-day workaround for adding the status code in an extension method:
|
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. |
You could optionally read the string result out of the response and into another key on the exception |
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. |
Are you aware that Exceptions are localized? Do you suggest to have a separate parser for every language? |
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. 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. |
@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 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;
} |
So a rough excerpt from a HttpClient Wrapper I have as an example.
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. |
We actually implemented this in PR #32455 for .NET 5. |
Duplicate of #911 |
When the
EnsureSuccessStatusCode
method in theHttpResponseMessage
throws aHttpRequestException
exception it is not possible to determine the status code.Add a new property named
StatusCode
to theHttpRequestException
class which gets set by theEnsureSuccessStatusCode
method of theHttpResponseMessage
class.Rationale and Usage
Allows you to determine the status code which triggered the exception.
Proposed API
Open Questions
Should the property have a setter?
Should the exception have a constructor that sets the property?
Should the property be nullable?
The text was updated successfully, but these errors were encountered: