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

[API Proposal]: WebSockets over HTTP/2 #69669

Closed
greenEkatherine opened this issue May 23, 2022 · 30 comments · Fixed by #70895
Closed

[API Proposal]: WebSockets over HTTP/2 #69669

greenEkatherine opened this issue May 23, 2022 · 30 comments · Fixed by #70895
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@greenEkatherine
Copy link
Contributor

greenEkatherine commented May 23, 2022

Background and motivation

Currently we are supporting WebSockets over HTTP/1.1 only. For WebSockets over HTTP/2 the protocol is quite different and described by RFC #8441. It is already supported by Chrome and there are an interest from YARP and ASP.NET partners. The main improvement is that supporting WebSockets over HTTP/2 will give advantage of multiplexing connection.

API Proposal

    // EXISTING
    class ClientWebSocket : WebSocket
    {
        // EXISTING
        public Task ConnectAsync(Uri uri, CancellationToken cancellationToken);
        // NEW
        public Task ConnectAsync(Uri uri, HttpMessageInvoker invoker, CancellationToken cancellationToken);
    }

    // EXISTING
    class ClientWebSocketOptions
    {
        // NEW
        public System.Version HttpVersion { get { throw null; } 
            [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
            set { } };
        public System.Net.Http.HttpVersionPolicy HttpVersionPolicy { get { throw null; } 
           [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
           set { } };
    }
    
    class HttpMethod : IEquatable<HttpMethod>
    {
        // EXISTING
        // internal static HttpMethod Connect { get { throw null; } }
        // NEW
        public static HttpMethod Connect { get { throw null; } }
    }

    class HttpRequestHeaders : HttpHeaders
    {
        public string? Protocol { get { } set { } };
    }

API Usage

var handler = new SocketsHttpHandler();

ClientWebSocket ws = new();
ws.Options.HttpVersionPolicy = HttpVersionPolicy.RequestVersionOrHigher;
ws.Options.HttpVersion = HttpVersion.Version20;

ws.ConnectAsync(uri, new HttpMessageInvoker(handler), cancellationToken);

HttpRequestMessage request = new(HttpMethod.Connect, server.Address);
request.Headers.Protocol = "websocket";

Notes

  • HttpVersion and HttpVersionPolicy in ClientWebSocketOptions are similar to HttpClient's property. ClientWebSocketOptions for browser is a class with completely different properties, so the new properties don't affect it.

  • Providing an external handler to ClientWebSocket is important because it enables advantage of HTTP/2 multiplexing by reusing the same handler for other ordinary HTTP/2 streams. Generic handler HttpMessageInvoker in ConnectAsync is a better alternative than SocketsHttpHandler. There is no need to check that it is SocketsHttpHandler but it should be able to handle WebSocket requests and it is up to the user who provides it.

  • The property for the :protocol header in HttpRequestHeaders is required because we need to guarantee that pseudo headers are appeared before all regular headers based on spec.

Other alternatives considered, but rejected:

  • Pass a low-level SocketsHttpHandler instead of HttpMessageInvoker.
  • Only one property HttpVersion if we do not require downgrade handling. In that case we rely on the version the user provides.

Risks

Adding new fields might increase memory usage.

@greenEkatherine greenEkatherine added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 23, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 23, 2022
@ghost
Copy link

ghost commented May 23, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Currently we are supporting WebSockets over HTTP/1.1 only. For WebSockets over HTTP/2 the protocol is quite different and described by RFC #8441. It is already supported by Chrome and there are an interest from YARP and ASP.NET partners. The main improvement that sup-porting WebSockets over HTTP/2 will give advantage of multiplexing connection.

API Proposal

    // EXISTING
    class ClientWebSocket : WebSocket
    {
        // NEW
        public System.Version DefaultRequestVersion { get { throw null; } set { } };
        public System.Net.Http.HttpVersionPolicy DefaultVersionPolicy { get { throw null; } set { } };

        // EXISTING
        public Task ConnectAsync(Uri uri, CancellationToken cancellationToken);
        // NEW
        public Task ConnectAsync(Uri uri, CancellationToken cancellationToken, SocketsHttpHandler sharedHandler);
    }


### API Usage

```csharp
var handler = new SocketsHttpHandler();

ClientWebSocket ws = new();
ws.DefaultVersionPolicy = HttpVersionPolicy.RequestVersionOrHigher;
ws.DefaultRequestVersion = HttpVersion.Version20;

ws.ConnectAsync(uri, cancellationToken, handler);

Alternative Designs

We may consider only one property DefaultRequestVersion if we do not require downgrade handling. In that case we rely on the version the user provides.

Risks

Adding new fields into ClientWebSocket object will increase memory usage.

Author: greenEkatherine
Assignees: -
Labels:

api-suggestion, area-System.Net, untriaged

Milestone: -

@greenEkatherine
Copy link
Contributor Author

Internal Changes

ClientWebSocket

To make downgrade for WebSocket from HTTP/2 to HTTP/1.1 possible we need to add additional header with the key because we cannot generate it later in HttpClient. Other headers can be easily added and renamed to construct HTTP/1.1 WebSocket connection request from HTTP/2 ws request.
HTTP/2 example:
image
HTTP/1.1 example:
image

HttpConnectionPool

Since YARP is using SocketsHttpHandler directly, we need to handle downgrade (if we decide to allow it) inside the HttpConnectionPool. It will check the new setting ID of the Http2Connection EnableConnect -- based on the server response.

To make things easier we also should consider the new property IsWebSocketRequest of HttpRequestMessage. It will check headers once to confirm that it is a websocket request (either h1 or h2) and we will not need to do it few times during downgrade.

@stephentoub
Copy link
Member

public System.Version DefaultRequestVersion { get { throw null; } set { } };
public System.Net.Http.HttpVersionPolicy DefaultVersionPolicy { get { throw null; } set { } };

Why "default"? We typically use that for statics providing a default value for any instance, but that's apparently not the case here.

    public Task ConnectAsync(Uri uri, CancellationToken cancellationToken, SocketsHttpHandler sharedHandler);

This is concerning. It ties ClientWebSocket to the underlying implementation that should be used. ClientWebSocket has historically used HttpWebRequest, WinHttpHandler, and SocketsHttpHandler, and even today has a browser-based implementation for wasm.

@CarnaViire
Copy link
Member

RequestVersion and VersionPolicy for connect request might look better as a part of ClientWebSocketOptions. But we might also want to have an additional HttpVersion property on ClientWebSocket itself if we support downgrade (so we could know what exactly was established)

@MihaZupan
Copy link
Member

It will check the new setting ID of the Http2Connection EnableConnect

Can you elaborate on what you meant here?

To make downgrade for WebSocket from HTTP/2 to HTTP/1.1 possible we need to add additional header with the key because we cannot generate it later in HttpClient. Other headers can be easily added and renamed to construct HTTP/1.1 WebSocket connection request from HTTP/2 ws request.

Would the downgrade be handled by the WebSocket layer (i.e. calling into the handler multiple times)?
Or did you mean that Websockets would be a special case the handler recognized to adapt headers as needed?

@CarnaViire
Copy link
Member

CarnaViire commented May 23, 2022

Would the downgrade be handled by the WebSocket layer (i.e. calling into the handler multiple times)?
Or did you mean that Websockets would be a special case the handler recognized to adapt headers as needed?

I believe both alternatives are considered, but for YARP it will be better if it was handled by HTTP layer

@greenEkatherine
Copy link
Contributor Author

I agree with @stephentoub and @CarnaViire suggestions to rename policy and version properties and move it into ClientWebSocketOptions:

    // EXISTING
    class ClientWebSocketOptions
    {
        // NEW
        public System.Version RequestVersion { get { throw null; } set { } };
        public System.Net.Http.HttpVersionPolicy VersionPolicy { get { throw null; } set { } };
    }

Would using HttpClient be better from that perspective?

    public Task ConnectAsync(Uri uri, CancellationToken cancellationToken, SocketsHttpHandler sharedHandler);

This is concerning. It ties ClientWebSocket to the underlying implementation that should be used. ClientWebSocket has historically used HttpWebRequest, WinHttpHandler, and SocketsHttpHandler, and even today has a browser-based implementation for wasm.

Providing an external handler is a keypoint here because in that case we are able to take advantage of HTTP/2 multiplexing by reusing the same handler for other ordinary HTTP/2 streams.

@stephentoub
Copy link
Member

Providing an external handler is a keypoint here because in that case we are able to take advantage of HTTP/2 multiplexing by reusing the same handler for other ordinary HTTP/2 streams.

Understood. It doesn't change it being problematic, though. What happens if we decide we no longer want ClientWebSocket to be implemented with SocketsHttpHandler (which is an implementation detail, and which isn't even used in all builds)? That's not theoretical; we've already made such a change at least twice before with this type.

We should think through alternatives. If we ship the proposed API, that's it, forever-more this is implemented with SocketsHttpHandler.

@greenEkatherine
Copy link
Contributor Author

It will check the new setting ID of the Http2Connection EnableConnect

Can you elaborate on what you meant here?

Based on the https://datatracker.ietf.org/doc/html/rfc8441#section-3 we should add a new SETTINGS parameter that a server sends to a client. My suggestion is to add a new property into Http2Connection to indicate this setting, so the HttpConnectionPool would be able to check its value of a connection and perform downgrade if needed.

@Tratcher
Copy link
Member

    public Task ConnectAsync(Uri uri, CancellationToken cancellationToken, SocketsHttpHandler sharedHandler);

This is concerning. It ties ClientWebSocket to the underlying implementation that should be used. ClientWebSocket has historically used HttpWebRequest, WinHttpHandler, and SocketsHttpHandler, and even today has a browser-based implementation for wasm.

It should at least be a HttpMessageInvoker.

@greenEkatherine
Copy link
Contributor Author

One more API change: HttpMethod.Connect is internal but to construct external ws request (i.e. in YARP) it should be public:

class HttpMethod : IEquatable<HttpMethod>
{
-        internal static HttpMethod Connect { get { throw null; } }
+        public static HttpMethod Connect { get { throw null; } }
}

However, we need somehow allow its external usage for websockets only.

@MihaZupan
Copy link
Member

The user can still create the method with new HttpMethod("CONNECT"), but I don't see a reason we shouldn't just expose the property. The comment about it in source seems incorrect - you should be able to use it directly too.

// Don't expose CONNECT as static property, since it's used by the transport to connect to a proxy.
// CONNECT is not used by users directly.

@greenEkatherine
Copy link
Contributor Author

greenEkatherine commented May 24, 2022

Next steps from the team discussion:

  1. Check how the new API are compatible with other handlers:
  • what is expected to happen in browser environment (e.g. mark is as unsupported so it throws PNSE and gets highlighted by the analyzer);
  • what will happen if you feed this "extended connect" request into HttpClient that has WinHttpHandler underneath;
  1. Discuss alternative overloading of ClientWebSocket.ConnectAsync with @stephentoub (HttpMessageInvoker or HttpMessageHandler or wait until HttpClient.Shared is implemented)

@Tratcher
Copy link
Member

I think HttpRequestMessage needs a new Protocol field to represent the new pseudo header. WebTransport also uses this field.

@greenEkatherine
Copy link
Contributor Author

I think HttpRequestMessage needs a new Protocol field to represent the new pseudo header. WebTransport also uses this field.

It would be great, I can reuse it to check if it's a WebSocket request.

@karelz karelz added this to the 7.0.0 milestone May 24, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 24, 2022
@greenEkatherine
Copy link
Contributor Author

greenEkatherine commented May 25, 2022

To sum up current API proposal:

    // EXISTING
    class ClientWebSocket : WebSocket
    {
        // EXISTING
        public Task ConnectAsync(Uri uri, CancellationToken cancellationToken);
        // NEW
        public Task ConnectAsync(Uri uri, HttpMessageHandler sharedHandler, CancellationToken cancellationToken);
    }

    // EXISTING
    class ClientWebSocketOptions
    {
        // NEW
        public System.Version RequestVersion { get { throw null; } set { } };
        public System.Net.Http.HttpVersionPolicy VersionPolicy { get { throw null; } set { } };
    }
    
    class HttpRequestMessage : System.IDisposable
    {
        // NEW
        public string? Protocol { get { } set { } };
    }

    class HttpMethod : IEquatable<HttpMethod>
    {
        // EXISTING
        // internal static HttpMethod Connect { get { throw null; } }
        // NEW
        public static HttpMethod Connect { get { throw null; } }
    }

@Tratcher
Copy link
Member

2. Discuss alternative overloading of ClientWebSocket.ConnectAsync with @stephentoub (HttpMessageInvoker or HttpMessageHandler or wait until HttpClient.Shared is implemented)

HttpMessageInvoker should be easier for customers to use than HttpMessageHandler, they likely already have an HttpClient instance that they want to share with. Otherwise they have to hold onto an HttpMessageHandler and share that around as well.

@greenEkatherine
Copy link
Contributor Author

Updated proposal:

    class ClientWebSocket : WebSocket
    {
        // EXISTING
        public Task ConnectAsync(Uri uri, CancellationToken cancellationToken);
        // NEW
        public Task ConnectAsync(Uri uri, HttpMessageInvoker sharedHandler, CancellationToken cancellationToken);
    }

    // EXISTING
    class ClientWebSocketOptions
    {
        // NEW
        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
        public System.Version Version { get { throw null; } set { } };
        [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
        public System.Net.Http.HttpVersionPolicy VersionPolicy { get { throw null; } set { } };
    }

    class HttpMethod : IEquatable<HttpMethod>
    {
        // EXISTING
        // internal static HttpMethod Connect { get { throw null; } }
        // NEW
        public static HttpMethod Connect { get { throw null; } }
    }

Summary based on discussions with the team:

  1. Version and VersionPolicy in ClientWebSocketOptions should be ok since it is similar to HttpClient's property. ClientWebSocketOptions for browser is a class with completely different properties, so the new properties don't affect it.

  2. Generic handler HttpMessageInvoker in ConnectAsync is a better alternative. There is no need to check that it is SocketsHttpHandler but it should be able to handle WebSocket requests and it is up to the user who provides it.

  3. The proper place to handle downgrade seems to be inside ClientWebSocket because otherwise we need to put a lot of ws-specific information inside HttpClient (like security key generation).

  4. Protocol property looks excessive at this point, should be enough to add header into the collection.

@stephentoub @Tratcher @CarnaViire @dotnet/ncl please let me know if I miss something

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2022

4. Protocol property looks excessive at this point, should be enough to add header into the collection.

The request header collection doesn't allow headers that start with ':' like ':protocol'. Will that be changed, or will this header be special cased?

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2022

3. The proper place to handle downgrade seems to be inside ClientWebSocket because otherwise we need to put a lot of ws-specific information inside HttpClient (like security key generation).

What does the downgrade flow look like? YARP will also need to implement this.

  1. Make an h2 CONNECT request
  2. It fails with some error (How to programmatically determine the error is about missing h2 Extended CONNECT support?)
  3. Retry with h1 Upgrade

@greenEkatherine
Copy link
Contributor Author

The request header collection doesn't allow headers that start with ':' like ':protocol'. Will that be changed, or will this header be special cased?

I tried to add this header for test purposes and didn't see issues with request. Also, we have ':authority' header there, is there any difference?

What does the downgrade flow look like? YARP will also need to implement this.

  1. Make an h2 CONNECT request
  2. It fails with some error (How to programmatically determine the error is about missing h2 Extended CONNECT support?)
  3. Retry with h1 Upgrade

In case of failed h2 CONNECT request HttpClient will return HttpRequestException. Should we consider different type of exception or clear error message would be enough?

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2022

What does the downgrade flow look like? YARP will also need to implement this.

  1. Make an h2 CONNECT request
  2. It fails with some error (How to programmatically determine the error is about missing h2 Extended CONNECT support?)
  3. Retry with h1 Upgrade

In case of failed h2 CONNECT request HttpClient will return HttpRequestException. Should we consider different type of exception or clear error message would be enough?

An error message is insufficient for targeted programmatic retries. There needs to be a unique derived exception type or some programmatically accessible state on the exception so we can identify the failure as the missing Extended CONNECT server setting (or lack of H2 ALPN?). We could take advantage of the Exception.Data collection if you don't want to add new public API. We wouldn't want to downgrade and retry for other types of failure like DNS or Socket issues.

@MihaZupan
Copy link
Member

MihaZupan commented Jun 7, 2022

We could take advantage of the Exception.Data collection if you don't want to add new public API

There is some related discussion in #43239 (comment) about extending the exception with protocol errors.
cc: @antonfirsov

@MihaZupan
Copy link
Member

I tried to add this header for test purposes and didn't see issues with request. Also, we have ':authority' header there, is there any difference?

If you try to add the :protocol header manually you will see an exception because the name is invalid (: is not a valid character for the name).

:authority is different because it's never added to the HttpHeaders collection explicitly. Instead, we generate it on the fly based on the host header when sending the request.

@greenEkatherine
Copy link
Contributor Author

We could take advantage of the Exception.Data collection if you don't want to add new public API.

I would prefer using of Exception.Data.

If you try to add the :protocol header manually you will see an exception because the name is invalid (: is not a valid character for the name).

Following offline discussion I have to return to the Protocol property in HttpRequestMessage. The issue is not only with formatting of pseudo-header :protocol, more important is the order of pseudo-headers from the spec:

All pseudo-header fields MUST appear in a field block before all regular field lines. Any request or response that contains a pseudo-header field that appears in a field block after a regular field line MUST be treated as malformed

With property we can guarantee the order. It also makes easier validation with reference to the protocol version because this header is not valid for HTTP/1.1.

@stephentoub what do you think about Protocol property in this context?

@stephentoub
Copy link
Member

stephentoub commented Jun 8, 2022

No other header has a property on HttpRequestMessage. If we really want/need a dedicated property, shouldn't it be on one of the headers collection types, where the other strongly-typed headers live, e.g. on HttpRequestHeaders?

@Tratcher
Copy link
Member

Tratcher commented Jun 8, 2022

HEADERS + END_HEADERS
:method = CONNECT
:protocol = websocket
:scheme = https
:path = /chat
:authority = server.example.com
sec-websocket-protocol = chat, superchat
sec-websocket-extensions = permessage-deflate
sec-websocket-version = 13
origin = http://www.example.com

:protocol is not a regular HTTP header, it's an h2 pseudo header. All other pseudo headers (method, scheme, path, authority) are derived from properties on the HttpRequestMessage. The same goes for :status on the HttpResponseMessage.

@stephentoub
Copy link
Member

Even so, I think it best maps to the HttpClient headers object model by being a part of the headers, e.g. request.Headers.Protocol rather than request.Protocol. I also like that doing so hides it a bit for HTTP/1.1 where it's irrelevant.

@greenEkatherine greenEkatherine added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 16, 2022
@greenEkatherine
Copy link
Contributor Author

Team reached agreement, updating the proposal and switching it to ready-for-api-review.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 17, 2022
@greenEkatherine greenEkatherine removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 21, 2022
@terrajobst
Copy link
Member

terrajobst commented Jun 28, 2022

Video

  • Looks good as proposed
namespace System.Net.WebSockets
{
    public partial class ClientWebSocket : WebSocket
    {
        // Existing
        // public Task ConnectAsync(Uri uri, CancellationToken cancellationToken);
        public Task ConnectAsync(Uri uri, HttpMessageInvoker invoker, CancellationToken cancellationToken);
    }

    public partial class ClientWebSocketOptions
    {
        public Version HttpVersion { get; [UnsupportedOSPlatform("browser")] set; };
        public HttpVersionPolicy HttpVersionPolicy { get; [UnsupportedOSPlatform("browser")] set; };
    }
}
namespace System.Net.Http
{
    public partial class HttpMethod
    {
        public static HttpMethod Connect { get; }
    }
}
namespace System.Net.Http.Headers
{
    public partial class HttpRequestHeaders : HttpHeaders
    {
        public string? Protocol { get; set; };
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 28, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants