Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2104\))
Subject: Re: Thoughts on the new Protocols architecture
From: Stefan Eissing <stefan.eissing@greenbytes.de>
In-Reply-To: <55FF85F0.8040106@gmail.com>
Date: Mon, 21 Sep 2015 14:00:00 +0200
Content-Transfer-Encoding: quoted-printable
Message-Id: <10CAC6C5-F3C1-44CE-B18F-2892E0F9423B@greenbytes.de>
References: <55FF85F0.8040106@gmail.com>
To: dev@httpd.apache.org

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>:
>=20
> =3D=3D Immediate Concerns =3D=3D
>=20
> 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
>=20
> =
https://github.com/jchampio/httpd/commit/26fba7261b2d1d194c693cf7206633766=
af56afb
>=20
> 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:
>=20
> =
https://github.com/jchampio/apache-websocket/blob/dev/protocol_switch/mod_=
websocket.c
>=20
> 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.
>=20
> 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.

> =3D=3D Medium-term (more WebSocket-specific) =3D=3D
>=20
> 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.

> =3D=3D Long-term/Idle Thoughts =3D=3D
>=20
> In no particular order:
>=20
> - 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? =46rom 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=C3=BCnster, Germany
Phone: +49 251 2807760. Amtsgericht M=C3=BCnster: HRB5782