Skip to content
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

HttpClient and WebSockets don't provide HTTP error details #19405

Closed
dlstucki opened this issue Nov 17, 2016 · 17 comments
Closed

HttpClient and WebSockets don't provide HTTP error details #19405

dlstucki opened this issue Nov 17, 2016 · 17 comments
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@dlstucki
Copy link
Contributor

I have a WebSocket service which can return error details in the HTTP response to a WebSocket upgrade request such as:

HTTP/1.1 404 Endpoint does not exist. TrackingId:71d07da7-8ace-4463-b764-1f76559aebd0_G2

When I use ClientWebSocket.ConnectAsync and it fails (expected) there's no way to get the HTTP StatusCode (e.g. 404) or the HTTP StatusDescription (e.g. "Endpoint does not exist. TrackingId:71d07da7-8ace-4463-b764-1f76559aebd0_G2").

@jtaubensee
Copy link

@karelz - Do you know if there is someone we can talk to about validating this issue? We'd like to get an idea if this is something that the team would like to fix at all (regardless of timeline), or if it is something we can help with a PR.

@karelz
Copy link
Member

karelz commented Nov 30, 2016

We are starting System.Net triage next Monday (see area list). It will likely drag till January -- the area is the largest (~20% of all corefx repo) and we lost our Linux/Mac expert recently.

That said, we can always speed up ad-hoc triage of particular issues if there's interest.
@stephentoub @CIPop @davidsh any ideas on the above?

@dstuckims if you have proposal for the API change, please let us know. Naively it looks like a reasonable ask.

@stephentoub
Copy link
Member

@stephentoub @CIPop @davidsh any ideas on the above?

Seems reasonable to me to expose some of those details somewhere, either on the exception that's thrown or on the ClientWebSocket instance itself. @davidsh and @CIPop should comment, though.

@CIPop
Copy link
Member

CIPop commented Nov 30, 2016

Sounds like a good idea.
We should first check if we've missed anything when reimplementing this over WinHTTP (before it may have included an inner WebException which had the status).

@dlstucki
Copy link
Contributor Author

In .NET 4.5 when ClientWebSocket.ConnectAsync fails like this it is actually possible to get the HTTP status code and description because the inner exception (which comes from HttpWebRequest) is a WebException and its Response property has an HttpResponse instance. If we could fabricate the same tree in the .NET Core codebase then the code I originally wrote on .NET 4.5 would work for .NET Core too!

image

@karelz
Copy link
Member

karelz commented Dec 1, 2016

What is the exception on .NET Core today?
Aligning it with full .NET Framework might be reasonable ask (unless we got rid of that on purpose).

@dlstucki
Copy link
Contributor Author

dlstucki commented Dec 1, 2016

With the WinHttp WebSocket implementation it's a WinHttpException (which is an internal only type) with no interesting details. With the Managed WebSocket implementation if the HTTP status line doesn't start with "HTTP/1.1 101 " (status code = 101, Switching Protocols) it simply throws a WebSocketException with a default message like this:

            throw new WebSocketException(SR.net_webstatus_ConnectFailure);

@CIPop
Copy link
Member

CIPop commented Dec 1, 2016

In .NET 4.5 when ClientWebSocket.ConnectAsync fails like this it is actually possible to get the HTTP status code and description because the inner exception

That's what I thought. Let's keep this issue to track an adaptation layer between WinHttpException and WebException.

(unless we got rid of that on purpose).

@karelz the WebException comes from the managed HTTP stack. It was replaced by an inner WinHttpException as a side effect of now using WinHTTP.

Not very sure but it may still be valuable for the WebException to have an internal of WinHttpException for even more details such as OS-level errors.

@karelz karelz changed the title System.Net.WebSockets.ClientWebSocket.ConnectAsync doesn't provide HTTP error details HttpClient and WebSockets don't provide HTTP error details Dec 14, 2016
@karelz
Copy link
Member

karelz commented Dec 14, 2016

Next steps: We need a plan to:

  1. Move WebException (and enum) from System.Net.Requests into System.Net.Primitives (we need Wes's ack)
  2. Use WebException in HttpClient (use WinHttpException as its inner exception)
  3. Add tests

@borrrden
Copy link

What is the status of this? It's a pretty big blocker for Couchbase Lite .NET because we need to present information to the user about why their connection failed (for example, invalid credentials) or possibly retry the connection if we feel the error is transient.

@karelz
Copy link
Member

karelz commented Jun 12, 2017

It is in Future milestone, so no promise when it will be addressed.
It is marked up-for-grabs, if you want to help, I can clear step [1] above. Just let me know.

@KLuuKer
Copy link

KLuuKer commented Sep 14, 2017

I am also using the WebException to check if it's a transient TLS error (we get these allot secretly)
So I am using these 2 status to check for possible transient error and just retrying the request after waiting 100 milliseconds
WebExceptionStatus.TrustFailure
WebExceptionStatus.SecureChannelFailure

Would really help to have those statuses back in some way or form, because when I get the error it's a direct result of a end user wanting to get some information from a external source (example starting a payment request) so I need to quickly retry any transient problem

@brogowski
Copy link

We are in need of this feature too.
I can write PR for this if there would be someone to guide me a little bit.

@amrmahdi
Copy link

Same issue here. The behavior is not consistent between .NET and .NET Core. The following code throws different exceptions on the 2 frameworks:

 var client = new ClientWebSocket();

 client.ConnectAsync(new Uri("wss://speech.platform.bing.com/speech/recognition/interactive/cognitiveservices/v1"), CancellationToken.None).GetAwaiter().GetResult();

Exception from .NET 462

Unhandled Exception: System.Net.WebSockets.WebSocketException: Unable to connect to the remote server ---> System.Net.WebException: The remote server returned an error: (403) Forbidden.
   at System.Net.HttpWebRequest.EndGetResponse(IAsyncResult asyncResult)
   at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization)

Exception from .NET Core 2.0

Unhandled Exception: System.Net.WebSockets.WebSocketException: Unable to connect to the remote server ---> System.Net.WebSockets.WebSocketException: Unable to connect to the remote server
   at System.Net.WebSockets.WinHttpWebSocket.ThrowOnInvalidConnectState()
   at System.Net.WebSockets.WinHttpWebSocket.<ConnectAsync>d__18.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.WebSockets.WebSocketHandle.<ConnectAsyncCore>d__20.MoveNext()
   --- End of inner exception stack trace ---
   at System.Net.WebSockets.WebSocketHandle.<ConnectAsyncCore>d__20.MoveNext()

As you can see, the exception from .NET Core does not indicate what is the problem is. As opposed to the exception from .NET 462, where the inner exception is the actual HttpWebRequest exception with the right error code.

@davidsh
Copy link
Contributor

davidsh commented Apr 8, 2018

This has been obsoleted since we no longer use WinHTTP for System.Net.WebSockets.Client and use SocketsHttpHandler for the HTTP stack.

@davidsh davidsh closed this as completed Apr 8, 2018
@amrmahdi
Copy link

amrmahdi commented Apr 9, 2018

@davidsh Even though the implementation changed to SocketHttpHandler, the main problem of not propagating an appropriate exception with the upgrade response still exits. See https://github.com/dotnet/corefx/blob/14435e14d39bf70697956535b8ac6c96b49e9cea/src/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs#L175 where it throws generic exception without any details. Should a new issue be opened tracking this issue ?

@davidsh
Copy link
Contributor

davidsh commented Apr 9, 2018

Yes, please open up a new issue. Please be specific in terms of what additional information you would like to see propagated to the exception and under what conditions.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests