Websocket sniffing doesn't work for Http2 compatible connections

[BrennanConroy]

In SignalR we use the presence of the IHttpWebSocketFeature to determine if we should tell the client that websockets is "available". The Http2Connection no longer sets the IHttpUpgradeFeature which the websockets middleware uses and this makes websocket sniffing fail so clients will fallback to SSE even though websockets is supported over Http1.

[BrennanConroy]

@Tratcher @halter73 @anurse @davidfowl

[Tratcher]

@muratg @Eilon this is a potential 2.2 RTM bug.

[Eilon]

Out of curiosity, could you argue that SignalR's sniffing is what's defective? It does seem a little odd to me to force the HTTP2 implementation to pretend like it might support upgrading, even though it absolutely never does and never will.

[Tratcher]

@Eilon it's a bit late to redesign that. We can revisit that in 3.0.

We've proposed using IApplicationBuilder.ServerFeatures for this kind of detection, but never developed the concept further.

[Eilon]

Have we anticipated any unintended side-effects of changing the HTTP2 implementation to pretend like it might support web sockets? I feel like in theory it would be safer to change SignalR, because that would by definition affect only SignalR. Like, have SignalR check:

bool ShouldSignalRTryWebSockets(Connection connection)
{
    if (connection is Http1)
        return ((HttpUpgrade)connection).IsUpgradable);
    else if (connection is Http2)
        return true;
    else
        return false;
}

And that way the ugliness is constrained to SignalR? That seems to be it would be a much smaller change and localized to only the affected area.

[halter73]

Theoretically, there could be servers that support HTTP/2 but not WebSockets, so that's probably not the best pattern to promote.

Also, even if the IsUpgradableRequest is false, SignalR currently assumes WebSockets is supported just by the presence of the IHttpUpgradeFeature on a non-WebSocket request.

[analogrelay]

I'd argue the current way WebSockets does this detection makes sense because there are three possible states:

1. The server is incapable of performing an Upgrade. (IIS/HttpSys on Windows 7, or HTTP2-only Kestrel)
2. The server is capable of Upgrade, but this request isn't an Upgrade request. (All HTTP/2 requests and HTTP requests without a Connection: Upgrade header)
3. The server is capable of Upgrade, and this request is an Upgrade request (HTTP/1 requests with a Connection: Upgrade header)

Those three states also map well to the behavior of the feature today:

1. The IHttpUpgradeFeature is not present.
2. The IHttpUpgradeFeature is present but IsUpgradableRequest is false.
3. The IHttpUpgradeFeature is present and IsUpgradableRequest is true.

I could totally see the benefit of completely removing the feature if Kestrel is configured for HTTP/2 only, since that falls in to category 1. However, Kestrel in Http2AndHttp1 mode, when processing a request, falls in to category 2.

Like, have SignalR check:

Wouldn't this cast require introducing a new dependency on Kestrel in SignalR? Seems more dangerous... Is there a way this is expressed through the abstraction?

[analogrelay]

I do think that a ServerFeature would be clearer and am 100% behind changing to that in 3.0 :)

[BrennanConroy]

If we decide to take the current change, I have tested it and it fixes the issue.

[Eilon]

Can SignalR check if the Kestrel bindings include an HTTP1.x endpoint, and so it should thus enable WebSockets?

[Tratcher]

@Eilon no, SignalR has no knowledge of Kestrel bindings.

[Eilon]

@DamianEdwards had mentioned that there might be some server features collection with some interface. Is that Kestrel-specific?

[Tratcher]

The server is incapable of performing an Upgrade. (IIS/HttpSys on Windows 7, or HTTP2-only Kestrel)

@anurse that wouldn't be handled by the current PR, kestrel always assumes it's capable of upgrades. Fixing it would also have to be per-endpoint aware which gets messy.

[Tratcher]

@Eilon that's the one I mentioned in https://github.com/aspnet/KestrelHttpServer/issues/3081#issuecomment-437105292, it has never been completely implemented. Today it does not have detailed endpoint information, only a list of addresses (IServerAddressesFeature).

[analogrelay]

@Tratcher totally agreed, that's something to be considered in 3.0 IMO.

[Eilon]

Hmm that's too bad. It just seems very ugly, and arguably wrong/misleading, to have the HTTP2 protocol seem to implement upgradability. And on top of that the fact that the problem is fundamentally in SignalR, not Kestrel.

[Tratcher]

Note it already works that way in HttpSys (and ANCM in proc?).

It's a capabilities detection issue that we never developed for the stack as a whole. Katana used to have one.

[halter73]

It would be easier to have Kestrel rather than SignalR check whether HTTP/1.x is enabled on the current endpoint. Kestrel could remove the IHttpUpgradeFeature from the request if HTTP/1.x is completely disabled, though I expect this will be an unusual configuration.

Doing this would complicate the fix slightly since it would be one of the only features in Kestrel that is selectively added to the feature collection. I also don't like how implicit this contract is, so I'd rather wait for 3.0 and provide a more though-out mechanism for detecting server WebSocket support.

[Eilon]

@davidfowl can you chime in on this issue with your thoughts? If possible, we'd like to fix this today, or else we'd need to wait and consider patching 2.2 if we feel it's high-pri.

[analogrelay]

It just seems very ugly, and arguably wrong/misleading, to have the HTTP2 protocol seem to implement upgradability

As a reminder, the presence of the IHttpUpgradeFeature is not intended to indicate that the current request can be upgraded. I don't really buy that it's misleading, since there's a boolean for exactly this reason. I do agree that the IHttpUpgradeFeature is probably an overloaded way to do this, and a server feature would be cleaner. The question is what do we think is the most appropriate fix for the time window. Our three options are basically:

1. Do nothing - SignalR will not use WebSockets in 2.2 when HTTP/2 is enabled (and it is disabled by default)
2. Change Kestrel to put the feature in (I'll leave the whole discussion of suppressing it in HTTP/2-only mode aside for now)
3. Change WebSockets to detect both of the following instead of checking for the IHttpUpgradeFeature: a) That current server is Kestrel without ANCM in in front b) That Kestrel is configured to allow HTTP/1 requests

If we can find a relatively clean way to isolate this change to the WebSockets code, then I don't mind doing it there, but it means diving pretty deep into Kestrel internals to figure it out and it means making decisions not based on available features but on the specific server in use. We'd have to continue doing the IHttpUpgradeFeature-based detection anyway, when not using Kestrel, to avoid a breaking change.

[Tratcher]

3 isn't really an option, there's no external visibility into kestrel's endpoint settings. Why do you mention "without ANCM in in front"? Only because HTTP/2 won't be relevant to the ANCM scenario?

[analogrelay]

I suppose if we kept the IHttpUpgradeFeature sniffing in place, and just added this hacky thing on top it wouldn't be necessary to detect ANCM. I was thinking about detection of the availability of WebSockets without using IHttpUpgradeFeature

[Eilon]

OK let's go ahead with the proposed change. I appreciate the discussion!

[Eilon]

(Approved for 2.2)