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

Better support for HTTP/2 in SignalR .NET client #42817

Merged
merged 7 commits into from
Aug 22, 2022

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Jul 19, 2022

We don't need to specify HTTP/1.1 for negotiate and sends anymore due to #17081 being fixed on all supported server versions!

This change also flows the HttpClient to the WebSocketTransport so that WebSockets over HTTP/2.0 can reuse the potentially already HTTP/2 connection.

I'm leaning towards not specifying webSocket.Options.HttpVersion = HttpVersion.Version20 by default and letting customers specify it via HttpConnectionOptions.WebSocketConfiguration. While the connection should fallback to HTTP/1.1 if HTTP/2 isn't supported, it would add additional connection startup latency in those cases.

Blocked on a few issues in Runtime around the new ClientWebSocket.ConnectAsync method.

Unfortunately, when using HTTP/2 websockets, settings like cookie and certs on the websocket options are ignored and the HttpClient settings are used instead. This is why the PR avoids passing the HttpClient to WebSocket.ConnectAsync for HTTP/1.1 currently. And we should probably document this behavior when setting HTTP/2.

@BrennanConroy BrennanConroy added area-signalr Includes: SignalR clients and servers feature-client-net Related to the SignalR .NET client labels Jul 19, 2022
@BrennanConroy BrennanConroy added the blocked The work on this issue is blocked due to some dependency label Jul 19, 2022
@BrennanConroy
Copy link
Member Author

Hmm, issue with this change is that now all the DelegatingHandler wrappers run for WebSocket requests, which is a problem in the AccessTokenHttpMessageHandler case when running in WASM due to headers not being settable. Not sure if the header would just be ignored, but regardless, the access token factory would be called again which would be unnecessary, unless we changed the handler to detect a WebSocket request and removed the access token factory call from WebSocketsTransport.

@BrennanConroy BrennanConroy marked this pull request as ready for review August 18, 2022 16:12
@BrennanConroy BrennanConroy removed the blocked The work on this issue is blocked due to some dependency label Aug 18, 2022
@BrennanConroy BrennanConroy changed the base branch from main to release/7.0-rc1 August 18, 2022 17:37
@halter73
Copy link
Member

How hard is it now to force HTTP/1.1 for all requests to get the old behavior if you really want to?

@BrennanConroy
Copy link
Member Author

In order to force HTTP/1.1 on all requests now (if for some reason ALPN negotiation isn't gracefully falling back to HTTP/1.1 or some other reason), you can write the following code:

private class Http11Handler : DelegatingHandler
{
    public Http11Handler(HttpMessageHandler handler)
        : base(handler)
    { }

    protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        request.Version = HttpVersion.Version11;
        return base.SendAsync(request, cancellationToken);
    }
}

HubConnection CreateConnection()
{
    return new HubConnectionBuilder().WithUrl(url, options =>
    {
        options.HttpMessageHandlerFactory = handler =>
        {
            return new Http11Handler((HttpClientHandler)handler);
        };
    }).Build();
}

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it might take something difficult like a DelegatingHandler to revert to the old behavior, but at least there's a way. Hopefully nobody will need to.

@adityamandaleeka adityamandaleeka added the blog-candidate Consider mentioning this in the release blog post label Aug 19, 2022
@ghost
Copy link

ghost commented Aug 19, 2022

@BrennanConroy, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

@BrennanConroy
Copy link
Member Author

@adityamandaleeka Additional testing added. Approve and merge at will 😃

@adityamandaleeka
Copy link
Member

Thanks @BrennanConroy!

@adityamandaleeka adityamandaleeka merged commit 9c8ad73 into release/7.0-rc1 Aug 22, 2022
@adityamandaleeka adityamandaleeka deleted the brecon/ws-h2 branch August 22, 2022 16:48
@BrennanConroy BrennanConroy added this to the 7.0-rc1 milestone Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers blog-candidate Consider mentioning this in the release blog post feature-client-net Related to the SignalR .NET client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants