httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Eissing <stefan.eiss...@greenbytes.de>
Subject Re: Thoughts on the new Protocols architecture
Date Mon, 21 Sep 2015 12:00:00 GMT
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 <champion.p@gmail.com>:
> 
> == 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




Mime
View raw message