[API Proposal] ClientWebSocket upgrade response details

[amrmahdi]

Updated by @CarnaViire

Background and motivation

ClientWebSocket currently doesn't provide any details about upgrade response. However, the information about response headers and status code might be important in both failure and success scenarios.

In case of failure, the status code can help to distinguish between retriable and non-retriable errors (server doesn't support web sockets at all vs just a small transient error). Headers might also contain additional information on how to handle the situation.

The headers are also useful even in case of a success web socket connect, e.g. they can contain a token tied to a session, or some info related to subprotocol version, or that the server can go down soon, etc.

There are three asks on GH for this ATM with a total of 21 distinct upvotes — #28331, #62474 and #25918 (this issue).

API Proposal

There are two alternatives, that are usable regardless of success/failure scenario, and also both opt-in, in order not to regress the existing usages in size and perf.

Option 1. ConnectAsync overload with a result object to fill in

// NEW
class WebSocketConnectResult
{
    public int? HttpStatusCode { get; set; }
    public IReadOnlyDictionary<string, IEnumerable<string>>? HttpResponseHeaders { get; set; }
}

// EXISTING
class ClientWebSocket
{
    // EXISTING
    public Task ConnectAsync(Uri uri, CancellationToken cancellationToken);
    // NEW
    public Task ConnectAsync(Uri uri, WebSocketConnectResult result, CancellationToken cancellationToken);
}

Usage:

ClientWebSocket ws = new();
WebSocketConnectResult result = new();
try
{
    await ws.ConnectAsync(uri, result, default);
    // success scenario
    ProcessSuccess(result.HttpResponseHeaders);
}
catch (WebSocketException)
{
    // failure scenario
    if (connectResult.HttpStatusCode != null)
    {
        ProcessFailure(result.HttpStatusCode, result.HttpResponseHeaders);
    }
}

Pros:

• Provides data that is essentially a connect result, from an overload of ConnectAsync method
• User can reuse the result objects
• Result object is independent from the ClientWebSocket object and its ownership/lifetime is "naturally" in hands of the user

Cons:

• Need to manually create additional object (no out var syntax here)
• User needs to ensure thread-safety, especially if reusing result objects

Option 2. WebSocket property with opt-in setting

// EXISTING
class ClientWebSocketOptions
{
    // NEW
    public bool CollectHttpResponseDetails { get; set; } = false;
}

// EXISTING
class ClientWebSocket
{
    // EXISTING
    public string? SubProtocol { get; }
    // NEW
    public int? HttpStatusCode { get; }
    public IReadOnlyDictionary<string, IEnumerable<string>>? HttpResponseHeaders { get; set; } // setter to clean up when not needed anymore
}

Usage:

ClientWebSocket ws = new();
ws.Options.CollectHttpResponseDetails = true;
try
{
    await ws.ConnectAsync(uri, default);
    // success scenario
    ProcessSuccess(ws.HttpResponseHeaders);
    ws.HttpResponseHeaders = null; // clean up (if needed)
}
catch (WebSocketException)
{
    // failure scenario
    if (ws.HttpStatusCode != null)
    {
        ProcessFailure(ws.HttpStatusCode, ws.HttpResponseHeaders);
    }
}

Pros:

• SubProtocol, which also can be treated as "connect result", is already a part of ClientWebSocket object
• No additional objects

Cons:

• Expanding ClientWebSocket object leads to increased memory footprint
• Even though it is possible to clean up by setting headers to null, user needs to be aware of that approach.

Other alternatives considered, but rejected:

• Returning result object from the method itself (Task<WebSocketConnectResult>) would either only work in success scenario or will require unconventional handling of every possible exception by storing it into the result object
• Having different approaches for success and failure scenarios doubles the API changes needed and is harder to use, plus adding a new derived exception is bad for discoverability.
• Using full HttpResponseMessage is dangerous, it is easy to misuse (and end up breaking the protocol/aborting connection by disposing/etc)
• In general, we want to distance from System.Net types in favor of plain types like int and IDictionary to enable wider usage.

---

Original post by @amrmahdi

Related to #19405

ClientWebSocket on .NET Core does not provide the upgrade request errors in the exception details as it does on the .NET Framework.

Repro code

var client = new ClientWebSocket();

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

Behavior on .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)
--- 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.ClientWebSocket.<ConnectAsyncCore>d__21.MoveNext()
   --- End of inner exception stack trace ---
   at System.Net.WebSockets.ClientWebSocket.<ConnectAsyncCore>d__21.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.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at ConsoleApp1.Program.Main(String[] args)

Behavior on .NET Core 2.1 preview 2

   Unhandled Exception: System.Net.WebSockets.WebSocketException: Unable to connect to the remote server
   at System.Net.WebSockets.WebSocketHandle.ConnectAsyncCore(Uri uri, CancellationToken cancellationToken, ClientWebSocketOptions options)
   at System.Net.WebSockets.ClientWebSocket.ConnectAsyncCore(Uri uri, CancellationToken cancellationToken)
   at ConsoleApp1.Program.Main(String[] args)

As you can see on .NET 462, the inner exception is a WebException with the error details.

Proposed Fix

Create an inner exception of type WebException in a similar fashion to
https://github.com/dotnet/corefx/blob/6acd74dda7bc4f585d2c4006da4a8b2deb0261ad/src/System.Net.Requests/src/System/Net/HttpWebRequest.cs#L1211
and throw if the response is not 200.

The original WebException in .NET framework was thrown fromHttpWebRequest.GetResponseAsync(), so I think the exception needs to be bubbled up in a similar way.

[davidsh]

This current PR dotnet/corefx#29159 should address most of this.

[amrmahdi]

Can we expose the WebException ? It has useful information like the response headers that the current PR is not addressing.

[davidsh]

Can we expose the WebException ? It has useful information like the response headers that the current PR is not addressing.

The .NET Core WebSockets.Client APIs use the new HTTP stack, SocketsHttpHandler. It does not use WebException. That exception is considered part of the legacy set of APIs including HttpWebRequest etc.

But we will look into how it might be possible to expose the HTTP response itself (HttpResponseMessage if available) which you could then use to examine the HTTP response headers.

cc: @geoffkizer @stephentoub @karelz

[amrmahdi]

Exposing HttpResponseMessage would be ideal.

[RaviPidaparthi]

It would be great if there is built in way we can read the websocket upgrade success and failure response headers.

[davidfowl]

As an added request since we're going down this rabbit hole. It would be great if we could also specify HttpMessageHandlers that run as part of the upgrade request so we don't need to special case WebSocketOptions.

[karelz]

@davidfowl can you please be more specific? How do you special case WebSocketOptions today? How do you plan to use HttpMessageHandlers?
How is it related to error details for upgrade request?

[davidfowl]

@davidfowl can you please be more specific? How do you special case WebSocketOptions today? How do you plan to use HttpMessageHandlers?

SignalR supports long polling, server sent events and websockets. We use HttpClient for the first 2 scenarios and ClientWebSocket for WebSockets. We lets users provide an HttpMessageHandler to do run arbitrary code before outgoing requests are sent with the HttpClient but that doesn't apply when using ClientWebSocket. The net effect is that we have to split out logic between configuring the HttpClientHandler, HttpMessageHandler and WebSocketOptions.

We have our own HttpConnectionOptions:

https://github.com/aspnet/SignalR/blob/679225c241b32c8e2af8c84fa693d3f8232d1085/src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnectionOptions.cs#L14

We have to manually apply them in both cases:

For HttpClient:

https://github.com/aspnet/SignalR/blob/679225c241b32c8e2af8c84fa693d3f8232d1085/src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnection.cs#L363

For ClientWebSocket:

https://github.com/aspnet/SignalR/blob/679225c241b32c8e2af8c84fa693d3f8232d1085/src/Microsoft.AspNetCore.Http.Connections.Client/Internal/WebSocketsTransport.cs#L47

How is it related to error details for upgrade request?

The upgrade request for websockets is the only time it's a regular HTTP request. As such, all existing things that work on http should apply. Right now we have to special case websockets we invented a whole new API that doesn't expose any of our existing HTTP stack (ClientWebSocket).

[qmfrederik]

Another use case for this: the Kubernetes API server has a bunch of WebSocket endpoints which take parameters via the query string.

If you specify a wrong value for any of those parameters, the server will respond with a 400 Bad Request and a JSON-formatted status object in the body which contains additional information.

The current design doesn't allow us to extract that information.

[dlstucki]

It's unfortunate the original report of this problem, from 2016, was closed because it happened to show a call stack from WinHttp. There were a number of "me toos" on that thread. Now it has the appearance of half as many people are asking for it and only being 17 days old.

[dlstucki]

PR dotnet/corefx#29159 just formats the status code in the exception.Message string. Are users supposed to parse that string in order to find the status code? I need to convert these into different exception types: e.g. 401->AuthorizationFailedException, 404->EndpointNotFoundException, and so on. Parsing an error message string feels like a work-around.

Could we put an HttpResponseMessage with status code, reason phrase, and response headers on the ClientWebSocket or on its ClientWebSocketOptions?

[dlstucki]

It would be great to expose the HttpResponseMessage, if present, in the resulting WebSocketException. I'd be interested in helping create a PR for this but the API would need to be determined.

Given this code which already has an HttpResponseMessage and throws the WebSocketException how should the response details be propagated in the Exception? A new property, HttpResponseMessage WebSocketException.HttpResponse { get; }? WebSocketContext WebSocketException.Context { get; }? (Putting the HttpResponse in the Exception.Data dictionary seems like a hack.)

    if (response.StatusCode != HttpStatusCode.SwitchingProtocols)
    {
        // **** TODO: put response in the Exception somewhere ****
        throw new WebSocketException(
            WebSocketError.NotAWebSocket,
            SR.Format(SR.net_WebSockets_Connect101Expected, (int)response.StatusCode));
    }

@karelz I would gladly create the PR if the API/mechanism of passing the HttpResponseMessage (or StatusCode, StatusDescription, and HTTP Response Headers) is determined.

[karelz]

Triage: Would be really nice to get it done in 5.0 for diagnostics + react to the upvotes count here.

[karelz]

Looks like this is API addition -- per #25918 (comment) we should add property on exception.
Sadly, it is too late for 5.0, moving to 6.0 (and fixing labels to capture API addition).

[cmsteffey]

@karelz Is there any chance the team could try to get something like this into .NET 7?
I don't think it'd be too bad to build a small exception with one extra property of the HttpResponse, and set that as the InnerException of the one that's thrown. Then people could get response content, status codes, headers, etc
(Correct me if I'm wrong)

[cmsteffey]

Thinking about it now, in regards to my above comment, there could actually be properties on the ClientWebsocket to get the UpgradeRequest and UpgradeResponse :)
Instead of trying to run the data through the exception
Just a thought

[karelz]

@cmsteffey it would be nice to have it, however I can't promise it for .NET 7 as we are fairly overbooked already :(

One of the planned features in .NET 7 is WebSockets for HTTP/2, so perhaps we could do it then (as we will be touching the code anyway and likely we will be adding new APIs) - @greenEkatherine @CarnaViire FYI.

[cmsteffey]

@karelz sounds good! If you want me to help, I'd love to
We might have to implement it in the underlying WebSocketHandle though which could be more difficult, due to ConnectAsync being run through the ConnectedWebSocket prop object

[CarnaViire]

There are largely two parts to this, as I see -- exposing HTTP headers on successful connect, and exposing HTTP status code and headers on an unsuccessful connect. The APIs I have in mind are as follows:

Exposing HTTP headers on successful connect

public class ClientWebSocket
{
    // EXISTING
    public Task ConnectAsync(Uri uri!!, CancellationToken cancellationToken);
    // NEW
    public Task ConnectAsync(Uri uri!!, CancellationToken cancellationToken, out HttpResponseHeaders? httpResponseHeaders);
}

Having an overload to ConnectAsync would allow users to only opt-in to getting the headers when they need them.

Points for consideration:

1. HttpResponseHeaders vs HttpHeaders — IMO the user would mostly need an access to custom headers, not the predefined ones, so maybe HttpHeaders would suffice?
2. Thread safety — should be addressed by Make concurrent reads on HttpHeaders thread-safe #68115 ?
3. Copy vs reference?

Alternatives: store headers in a property in ClientWebSocket — this would grow the size of the object even when users would never need the headers, plus IMO the headers are needed only in couple with connect and don't bear much significance after.

Note on returning full HttpRequestMessage

Exposing full HttpRequestMessage is dangerous.
The stream that is used for WebSocket is HttpResponseMessage.Content stream. Exposing full response message would mean the user would have an unsupervised access to the stream used by the WebSocket. They might unknowingly break the protocol. They might also dispose HttpResponseMessage which would lead to aborting the connection. I also doubt that anything from the response message, beside status and headers, can be needed?

Exposing HTTP status code and headers on an unsuccessful connect

public class WebSocketConnectException : WebSocketException
{
    public WebSocketConnectException(string? message) : base(message) {}
    public WebSocketConnectException(WebSocketError error, string? message) : base(error, message) {}
    public WebSocketConnectException(WebSocketError error, string? message, Exception? innerException) : base(error, message, innerException) {}

    public HttpStatusCode? HttpStatusCode { get; } // internal set
    public HttpResponseHeaders? HttpResponseHeaders { get; } // internal set
}

Points for consideration:

1. I am not sure whether objects put into exceptions should be serializable?
2. WebSocketException is serializable, also it is derived from Win32Exception and has some type forwarding set up — does it impose any restrictions on a derived exception type?
3. WebSocketException is sealed — what is the policy for removing sealed?

I don't see it as a separate exception (not related to WebSocketException) because it would change existing logic (and users would have catches for WebSocketExceptions). Adding (nullable) properties to WebSocketException itself also would not look good, because it is part of base System.Net.WebSockets library (abstract from HTTP, separate from System.Net.WebSockets.Client library)

Alternatives: put the additional data to Exception.Data dictionary — no additions to API, but not strongly typed and bad for discoverability.

[MihaZupan]

Minor comment about

public Task ConnectAsync(Uri uri!!, CancellationToken cancellationToken, out HttpResponseHeaders? httpResponseHeaders);

We can't have an out on an async method. Given the existing overload, we can't just change the return type either.

[davidfowl]

making a property like ConnectResponseHeaders settable, and someone can null it out after they're done with it (or make the property instead a method that gets and clears)

I prefer this.

making it opt-in by adding to ClientWebSocketOptions a bool property that's false by default and that you set to true if you want these various Connect members to be filled in

This is tempting as it won't regress in .NET 7 but it's an extra API.

What about a hybrid? If you assign ConnectResponseHeaders to a non null value, then set the headers otherwise, it won't be. That allows for an opt-in model without the additional bool.

Thoughts?

[CarnaViire]

I believe that (especially hybrid approach) might be bad for discoverability.

Imagine you use ClientWebSocket, and at some point you realize you need to look at headers. You press '.' and see there's a property with headers. How convenient! But you access it and it is always null. Where are the headers, you ask.

Now if you think to look at the options, you can find the opt-in there, so you might end up successful. But without the property, it seems a bit counter-intuitive and you have no other way than to learn the way to use it from the docs.

That made me thinking, maybe we can have the option with opt-out instead. So if you are highly concurrent scenario and the size growth ended up being too much, you can go and opt out, or if you need the headers, then clean it up after using. Otherwise, if you just have one client, you wouldn't really notice anything, and if you need the headers, they would be easily discoverable.

(The cleaning up by setting the property to null also seems questionable.... as a user, how can you be sure you will not break anything, if you mess up with the properties of a library object that's currently in use?)

(In the end, I am still not convinced that this would be better than having a connect method with full result...)

[davidfowl]

That made me thinking, maybe we can have the option with opt-out instead. So if you are highly concurrent scenario and the size growth ended up being too much, you can go and opt out, or if you need the headers, then clean it up after using. Otherwise, if you just have one client, you wouldn't really notice anything, and if you need the headers, they would be easily discoverable.

This isn't great because it means things compiled for earlier versions of .NET that use websockets that run on .NET 7 will use more memory and the only way to fix it would be to have that library specifically target .NET 7 so it can set the property to false (this is what YARP would have to do).

I prefer something opt in.

[stephentoub]

Another approach:

public class WebSocketConnectionResult
{
    ...
}

public class ClientWebSocket
{
    public Task ConnectAsync(..., WebSocketConnectionResult result);
}

Basically, an overload of ConnectAsync that accepts from the caller the object to fill in. That's opt-in, an overload of the existing method, handles providing the data the same whether success or failure, etc. It could even be more efficient if we allow the caller to reuse the object.

[CarnaViire]

Thanks!
I believe now we are down to two alternatives -- both of them opt-in and both of them usable regardless of success/failure scenario:

Option 1. ConnectAsync overload with a result object to fill in

// NEW
class WebSocketConnectResult
{
    public int? HttpStatusCode { get; set; }
    public IDictionary<string, IEnumerable<string>>? HttpResponseHeaders { get; set; }
}

// EXISTING
class ClientWebSocket
{
    // EXISTING
    public Task ConnectAsync(Uri uri, CancellationToken cancellationToken);
    // NEW
    public Task ConnectAsync(Uri uri, CancellationToken cancellationToken, WebSocketConnectResult result);
}

Usage:

ClientWebSocket ws = new();
WebSocketConnectResult result = new();
try
{
    await ws.ConnectAsync(uri, default, result);
    // success scenario
    ProcessSuccess(result.HttpResponseHeaders);
}
catch (WebSocketException)
{
    // failure scenario
    if (connectResult.HttpStatusCode != null)
    {
        ProcessFailure(result.HttpStatusCode, result.HttpResponseHeaders);
    }
}

Pros:

• Provides data that is essentially a connect result, from an overload of ConnectAsync method
• User can reuse the result objects
• Result object is independent from the ClientWebSocket object and its ownership/lifetime is "naturally" in hands of the user

Cons:

• Need to manually create additional object (no out var syntax here)
• User needs to ensure thread-safety, especially if reusing result objects

---

Option 2. WebSocket property with opt-in setting

// EXISTING
class ClientWebSocketOptions
{
    // NEW
    public bool CollectHttpResponseDetails { get; set; } = false;
}

// EXISTING
class ClientWebSocket
{
    // EXISTING
    public string? SubProtocol { get; }
    // NEW
    public int? HttpStatusCode { get; }
    public IDictionary<string, IEnumerable<string>>? HttpResponseHeaders { get; set; } // setter to clean up when not needed anymore
}

Usage:

ClientWebSocket ws = new();
ws.Options.CollectHttpResponseDetails = true;
try
{
    await ws.ConnectAsync(uri, default);
    // success scenario
    ProcessSuccess(ws.HttpResponseHeaders);
    ws.HttpResponseHeaders = null; // clean up (if needed)
}
catch (WebSocketException)
{
    // failure scenario
    if (ws.HttpStatusCode != null)
    {
        ProcessFailure(ws.HttpStatusCode, ws.HttpResponseHeaders);
    }
}

Pros:

• SubProtocol, which also can be treated as "connect result", is already a part of ClientWebSocket object
• No additional objects

Cons:

• Expanding ClientWebSocket object leads to increased memory footprint
• Even though it is possible to clean up by setting headers to null, user needs to be aware of that approach.

---

Other alternatives considered, but rejected:

• Returning result object from the method itself (Task<WebSocketConnectResult>) would either only work in success scenario or will require unconventional handling of every possible exception by storing it into the result object
• Having different approaches for success and failure scenarios doubles the API changes needed and is harder to use, plus adding a new derived exception is bad for discoverability.
• Using full HttpResponseMessage is dangerous, it is easy to misuse (and end up breaking the protocol/aborting connection by disposing/etc)
• In general, we want to distance from System.Net types in favor of plain types like int and IDictionary to enable wider usage.

---

cc @stephentoub @davidfowl @dotnet/ncl
Does this sound all right? Let me know if you have any concerns or additional ideas, otherwise I would proceed with this proposal.

[CarnaViire]

I've updated the top post with the proposal.

[MihaZupan]

Nit: CT should be the last parameter

-public Task ConnectAsync(Uri uri, CancellationToken cancellationToken, WebSocketConnectResult result);
+public Task ConnectAsync(Uri uri, WebSocketConnectResult result, CancellationToken cancellationToken);

[MihaZupan]

Also should the headers be an IReadOnlyDictionary?

[CarnaViire]

Also should the headers be an IReadOnlyDictionary?

That would make sense, I was also thinking about that recently 👍

[CarnaViire]

Team reached agreement on the updated proposal (in the top post). Marking as ready for review.

[CarnaViire]

Hi @cmsteffey, you've previously expressed interest in helping us with the implementation. We are currently still going through the API review process, but once it's finalized, would it be something you're still interested in?

[bartonjs]

Video

We generally liked option (2) over option (1).

• We don't particularly feel like the nullable return value on the HttpStatusCode is needed, the code should always be populated and can use a sentinel value for "I haven't attempted connecting yet"
• The HttpStatusCode property seems like it should be using System.Net.HttpStatusCode instead of int as its return type. If there's a layering problem, or other technical constraint, then int is OK.

namespace System.Net.WebSockets
{
    public partial class ClientWebSocketOptions
    {
        public bool CollectHttpResponseDetails { get; set; } = false;
    }

    public partial class ClientWebSocket
    {
        // EXISTING
        //public string? SubProtocol { get; }

        // NEW
        public System.Net.HttpStatusCode HttpStatusCode { get; }
        public IReadOnlyDictionary<string, IEnumerable<string>>? HttpResponseHeaders { get; set; } // setter to clean up when not needed anymore
    }
}