Closed wtn closed 1 year ago
Upgrading a connection is HTTP version specific. HTTP/0.9 - HTTP/1.1 use the upgrade: websocket
response header to indicate the upgrade was successful while HTTP/2 uses :protocol: websocket
pseudo header.
Because of this, we need to generate a HTTP version specific response.
In Async::WebSocket
, this is handled by the bifurcation UpgradeRequest
/UpgradeResponse
and ConnectRequest
/ConnectResponse
(source).
To avoid having to specify these headers explicitly, Protocol::HTTP::Request#protocol
and Protocol::HTTP::Response#protocol
exist. There are some subtle differences:
Response#protocol
. To accept the "upgrade" with selected protocol, you must respond with 101 Switching Protocols and a suitable upgrade header.Response#protocol
. To accept the "connect" with protocol, you just response with 200 OK.So, the reason for the Request#protcol
and Response#protocol
is to hide some of this complexity from the user. However it's not a perfect abstraction as explained above the actual behaviour is quite different between HTTP/1 and HTTP/2.
This is why you get such a warning: Protocol::Rack::Response
does not want you setting headers that affect the underlying protocol, and instead wants you to use the metadata fields it understands, which it will map to the underlying version specific behaviour.
In this case, what you want is "rack.protocol" => "websocket"
as a response header. If you are interested, this is discussed in more detail here: https://github.com/rack/rack/issues/1946 however I have not had success getting other servers on board, yet.
There are tests here which validate the correct behaviour of a wide range of servers: https://github.com/socketry/rack-conform/blob/main/test/rack/conform/websocket.rb however this will only work on Rack 3+.
In terms of your code, the Response#protocol
is set as the last argument here: https://github.com/wtn/async-websocket/commit/672d54bf1f0af5828c7e16775b588053700508f5#diff-73bff7d916a1d7037490608f2accca552ba2eb36ee8a3e5dbd119e359c21d081R30
The way that gets mapped back out is by Protocol::Rack
here: https://github.com/socketry/protocol-rack/blob/708582d65242ee0d1a512073a631e798a49f03ba/lib/protocol/rack/adapter/rack2.rb#L125-L128
The best behaviour would be for puma
to detect this and set the appropriate connection
and upgrade
headers (as outlined in the above issue for Rack).
If you are running on Puma (and Rack 3+), the following compatibility middleware should work:
class RackProtocolToConnectionUpgrade
def initialize(app)
@app = app
end
def call(env)
status, headers, body = @app.call(env)
if protocol = headers.delete('rack.protocol')
headers['upgrade'] = protocol
headers['connection'] = 'upgrade'
end
return status, headers, body
end
end
I only eyeballed the code, but this should be the gist of it. If you are on Rack 2.x, you need to use Rack::Utils::HeaderHash
to extract the headers.
Thank you for your thorough explanation.
I can confirm that the compatibility middleware resolves the issue. For safety, I'm only setting the headers when env['SERVER_PROTOCOL']
is HTTP/1.1.
Now the concept described in rack/rack#1946 totally makes sense. I agree in principle that the app server (Puma, Falcon, WEBrick, etc.) should be required to manage connection
and other hop-by-hop header fields. So I support making rack.protocol
a required part of the Rack spec.
Please add your feedback to that issue if you have time.
I'm happy this fixed your issue.
I've been using
Async::Websocket::Adapters::Rails
(source) with Puma and it's very useful. But when Puma is behind HAProxy, WebSocket connection negotiation fails with status code 502.I found this HAProxy blog post, which provides the following guidance:
This requirement agrees with RFC 7231 section 6.2.2, which says
The MDN Web Docs have a note that
I added both headers to the HTTP/1.x upgrade response in https://github.com/wtn/async-websocket/commit/672d54bf1f0af5828c7e16775b588053700508f5, and that resolved the problem.
However, this change resulted in warnings when running the tests, so I opened this issue to discuss.