Jacob, thanks a lot for reviewing/implementing it. Having another module use it helps seeing the current weaknesses. Detailed comments inline:
> Am 21.09.2015 um 06:22 schrieb Jacob Champion <[email protected]>: > > == Immediate Concerns == > > 1) WebSocket requires a handshake to be made with the 101 response, which > means a module that implements the protocol_switch hook needs to be able to > affect the response headers, as well as abort the upgrade with a 4xx/5xx if > necessary. To do this I added a hook that I called pre_protocol_switch; you > can take a look at the patch at > > https://github.com/jchampio/httpd/commit/26fba7261b2d1d194c693cf7206633766af56afb > > I also ported mod_websocket to use this new experimental hook along with the > other Protocols-related hooks, so you can see what a module using it might > look like: > > https://github.com/jchampio/apache-websocket/blob/dev/protocol_switch/mod_websocket.c > > I'm not sure I like my new hook, to be honest. I think I may have made it too > general with the RUN_ALL hook implementation and I'm not convinced that more > than one module needs to be able to affect an Upgrade handshake. I also don't > like the brittle coupling the mod_websocket implementation has: when the > module responds to pre_protocol_switch, it takes several actions (mutex > locking, subprotocol negotiation, allocation) with the assumption that it > will be the module handling the protocol_switch hook. If another module > steals the switch, we'll be left with leaked mutexes and probably hung client > plugins. > > But I haven't come up with a better idea yet. A corresponding > post_protocol_switch hook _might_ help with cleanup in the general case, but > I don't think mod_websocket's implementation could make use of it. Perhaps a > hook is not the right way to do what I'm trying to do. I think a "protocol_ugprade" hook would work better. That way, a module like yours can replace the core upgrade handling itself, send your own 101 response and call ap_switch_protocol() yourself. Would that work for you? > 2) WebSocket requires the Upgrade protocol to be handled case-insensitively > (`Upgrade: websocket` and `Upgrade: WebSocket` are both valid), but the > current protocol_propose implementation dance is case-sensitive. I hacked > ap_array_str_index to use strcasecmp just to get it to work on my end, but it > made me wonder: are Upgrade header values case-sensitive as a general rule? I > wasn't able to find any statement in RFC 7230 on this, unlike the rule for > Connection header values (they're case-insensitive). Yes, 7230 explicitly mentions header values that are to be treated case-insensitive and "Upgrade:" is not among them. However WebSocket spec does, hmm. Unfortunate. 7230 speaks and has registered "HTTP/x(.x)?" as token, but for ALPN it is "http/1.1" (and not case-insensitive), I tend to propose that we treat tokens in "Upgrade:" headers as case-insensitive. That would mean: The core upgrade handler would lower case all protocol tokens in the Upgrade header before passing it to ap_select_protocol(). That makes selections normalized again. If someone wants to see the original value, it needs to look at the request headers itself. > == Medium-term (more WebSocket-specific) == > > 1) WebSocket prefers that the server close the TCP connection before the > client, and if I don't do an explicit close before returning from > protocol_switch, Autobahn fails my test cases because the server is > apparently waiting for the client to close first. But if I close the > connection explicitly, mpm_event complains about a "network write failure in > core output filter", so that doesn't seem good either. Is there a "proper" > way to do this on my end, or is there more architectural work that would be > needed to support server-led TCP closes? Hmm, you should have to do nothing. The core upgrade handler marks the connection for closing which *should* be enough. Bug? > 2) Protocols is per-virtual-host, but I think typically people want to enable > WebSocket for a particular URI, not an entire server. And it doesn't make > much sense to me to set a "preference" for WebSocket vs. HTTP/2 in the > Protocols directive -- they're doing entirely different things, and I don't > think a client is likely to send them both in the same upgrade request > anyway. Maybe WebSocket is just an oddball protocol from Upgrade's point of > view? Yes and no. Different from http/2 as that is another framing layer that still has http/1.1 semantics. But also a client in HTTP/1.1 Upgrades. The mechanism we implement should work for both. > == Long-term/Idle Thoughts == > > In no particular order: > > - The access_log is only updated (with the 101 status) once the entire > connection comes to an end, which seems undesirable. Ok, needs to be fixed. > - h2_session_process seems conceptually similar to mod_websocket's framing > loop, which I think is a good thing. We seem to have settled on the same way > to solve the "read and write simultaneously" problem. > - How does mod_h2 interact with MPMs? From a first reading it seemed like it > would always split requests into separate threads. Are there any plans to > have it use different strategies according to which MPM is installed? (In > particular, I'm hoping that one WebSocket connection won't necessarily have > to steal an entire thread from httpd, and that MotorZ might help us out > here...) Not having a separate worker pool for mod_h2 is desirable. However for 2.4.x branch, there are limits to what can be done. In trunk, integration should happen. Currently I know not enough about mpms to come up with a solid proposal. //Stefan <green/>bytes GmbH Hafenweg 16, 48155 Münster, Germany Phone: +49 251 2807760. Amtsgericht Münster: HRB5782
