[API Proposal] ClientWebSocket upgrade response details

@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.

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

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

@geoffkizer

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

Exposing HttpResponseMessage would be ideal.

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

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.

@davidfowl

@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).

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.

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.

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?

@karelz

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.

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

Also should the headers be an IReadOnlyDictionary?

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

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

@cmsteffey

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?

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

Navigation Menu

Repository navigation

[API Proposal] ClientWebSocket upgrade response details #25918

Background and motivation

API Proposal

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

Option 2. WebSocket property with opt-in setting

Other alternatives considered, but rejected:

Repro code

Behavior on .NET 462

Behavior on .NET Core 2.1 preview 2

Proposed Fix

30 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[API Proposal] ClientWebSocket upgrade response details #25918

Description

Background and motivation

API Proposal

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

Option 2. WebSocket property with opt-in setting

Other alternatives considered, but rejected:

Repro code

Behavior on .NET 462

Behavior on .NET Core 2.1 preview 2

Proposed Fix

Activity

davidsh commented on Apr 17, 2018

amrmahdi commented on Apr 17, 2018

davidsh commented on Apr 17, 2018

amrmahdi commented on Apr 17, 2018

RaviPidaparthi commented on Apr 17, 2018

davidfowl commented on Apr 18, 2018

karelz commented on Apr 18, 2018

davidfowl commented on Apr 18, 2018

qmfrederik commented on Apr 30, 2018

dlstucki commented on May 5, 2018

dlstucki commented on May 7, 2018

dlstucki commented on Jun 4, 2019

karelz commented on Oct 1, 2019

30 remaining items

CarnaViire commented on May 10, 2022

CarnaViire commented on May 10, 2022

CarnaViire commented on May 10, 2022

bartonjs commented on May 19, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions