-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add HTTP/2 WebSockets support #362
Conversation
described in RFC 8441. A new directive 'H2WebSockets on|off' has been added. The feature is by default not enabled. As also discussed in the manual, this feature should work for setups using "ProxyPass backend-url upgrade=websocket" without further changes. Special server modules for WebSockets will have to be adapted, most likely, as the handling if IO events is different with HTTP/2.
error log in case of failure
…spects all files.
if ((rv = apr_pollset_add(tunnel->pollset, tunnel->origin->pfd))) { | ||
return rv; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the problem when the same/original strategy applies to both sockets and pipes?
It seems to me that a working POLLOUT on the client/pipe side is essential here, or what happens if the backend has a lot of data to send and the h2_c1 connection can't cope?
Doesn't h2 stop reading from the pipe when the h2_c1 would block, so that the pipe fills in and proxy_tunnel_transfer() can then stop reading from the origin socket to calm things down until POLLOUT is back on the pipe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, thanks for reviewing this.
POLLOUT on the fd will not work, imo. It is just for reading the signal that new input is available (pipes are in one direction). And you are correct that the strategy is not as good as the one with 2 sockets.
Passing a chunk from origin to client may block in the worker thread, but that does not block the TCP socket to the client in the main connection thread. Data from the client will be accepted, up to the HTTP/2 flow control window, and buffered. There is still a chance of messing up if the client does stop reading output on a stream. But if it really does not want any more output, it should RST the whole stream, I think. On a RST, our blocked output would CONNABORT.
Even if we add a second pipe for output, I have no good idea how POLLOUT signalling on that one would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we need two pipes, c1[out]->c2[in] for inbound and c2[out]->c1[in] for outbound (in/out is h1 point of vue, you may see it differently :) ).
Or maybe better a socketpair() which is bidirectional on both sides, available on unix/mac platforms and which we could emulate on Windows (something like the pollset wakeup_socket in APR which is using a pair of AF_INET sockets on 127.0.0.1, or we could use a pair of AF_UNIX sockets which seem to be implemented since Windows 10, e.g. https://gitlab.com/openconnect/openconnect/-/merge_requests/320).
If both h2 and h1 have their own socket to communicate with each other I think we can go full async/nonblocking on both sides (the socket is the buffering/limiting/pollable mechanism between the two), and both could even defer to the MPM for polling.
We could use the current ap_bucket_type_{request,response,headers} to exchange the HTTP header/trailers between h2 and h1 still (it's already parsed/bufferized/full so it would be unfortunate to use the socket/pipes for that), but the request/response bodies could be chunked and received/sent normally by the core/network filters on the h1 side, while h2 (mplx?) could have a pollset of all the h1 sockets/pipes to POLLIN/POLLOUT from/to or even defer to the MPM and be called back (ap_mpm_register_poll_callback_timeout).
Faster to talk about than implement though.. Does it sound like a (realistic) plan still?
Maybe you saw #294 (wip) already, there the h1 header fetching part is fully async already and on the mod_proxy_http side we are not very far from going async/nonblocking for everything (like we do for websocket in trunk already). I'm (slowly) moving on this..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are talking about a decent chunk of work. And I love the idea of making everything pollable and shifting it to mpm_event. However, that seems a long term thing.
For this PR, I am looking at adding H2 websockets to 2.4.x later and need to limit the scope just a tiny bit. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's do that for now then :)
So this hunk looks good, the proxy_tunnel_transfer() one below wants an ap_assert() IMHO (see there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the changes as suggested.
- proxy_tunnel_transfer() change error handling as suggested - use APR_ENOTIMPL as default return in new hook - use HTTP_SWITCHING_PROTOCOLS instead of 101 const - use apr_generate_random_bytes() and add error check on its return - use ap_cstr_casecmp instead of apr_strnatcasecmp throughout mod_http2 and mod_proxy_http2 - rename new API call/hook to ap_get_pollfd_from_conn() - add include apr_poll.h to http_core.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Added to trunk as r1910507. |
Adds support for bootstrapping WebSockets via HTTP/2, as described in RFC 8441.
A new directive 'H2WebSockets on|off' has been added. The feature is by default not enabled by default.
As also discussed in the manual, this feature should work for setups using
ProxyPass backend-url upgrade=websocket
without further changes. Special server modules for WebSockets will have to be adapted, most likely, as the handling if IO events is different with HTTP/2.Sample Setup
Assume you have a HTTP/1.1 WebSockets service sitting on
localhost
port5001
, and want to expose that underneath/ws
for HTTP/1.1 and HTTP/2 clients on yourmy-domain.org
site:Depending on what you do over those WebSockets, you might need to configure larger timeouts than for standard HTTP requests. There is a
timeout=seconds
parameter on theProxyPass
directive. And there isH2StreamTimeout
and the generalTimeout
for all connections to tweak.Update:
CI: due to github's ancient OS versions, we get a websockets python module installed that is "too old". I have no platform to debug this and no real will to. I added a check for a minimum version 10.4 of the module that is running on my debian sid. So, in github CI, the websockets tests in mod_http2 will skip for now.