I suppose you're right, it will short circuit anyway. So yah, I can move them up.
Brian On Friday, January 31, 2014, Leif Hedstrom <zw...@apache.org> wrote: > Thanks! > > > I need to check the patch again , but not sure I understand why it be more > expensive to move up the method and version checks (they would still be > after the presence checks). It also makes more logical sense to put all > prerequisites in one statement. > > Cheers, > > -- Leif > > > On Jan 31, 2014, at 5:48 PM, Brian Geffon <bri...@apache.org<javascript:;>> > wrote: > > > > See responses below. > > > >> On Fri, Jan 31, 2014 at 4:05 PM, Leif Hedstrom > >> <zw...@apache.org<javascript:;>> > wrote: > >> > >> > >>> On Jan 29, 2014, at 6:22 PM, Brian Geffon > >>> <bri...@apache.org<javascript:;>> > wrote: > >>> > >>> Hi All, > >>> I've created a patch adding WebSocket support to ATS, I would > appreciate > >>> community feedback. This is being tracked in TS-2541, the patch is > >> attached > >>> to the jira ticket https://issues.apache.org/jira/browse/TS-2541 > >> > >> > >> > >> Couple of nitpicks / observations: > >> > >> 1. You are adding some new connection metric. Do these count against > >> HTTP(S) connections as well? I'm assuming not, in which case, there > might > >> be some aggregation metrics (stats.config.xml) that should be updated > >> accordingly. > > > > Yes, since the connection is established as an HTTP connection it will be > > counted as an active http connection until the websocket connection is > > destroyed. The new metric allows you to drill down on just websocket > > connections. > > > > > >> > >> 2. A few more comments here and there (in HttpTransact.cc) might be > nice, > >> specially around the KA conditions ... ;) But it's fairly readable > already. > > > > I can try to add a few more comments to make the flow clearer. > > > > > >> > >> 3. In HttpTransact::handle_upgrade_request() why not check for the GET > >> method and HTTP/1.1 at the top ("quickest way" ...) ? > > > > The idea here is that we can determine that a connection is definitely > not > > an upgrade if it's missing an upgrade header or a connection header, > since > > both of those can be checked with just a bitwise and because of the > > presence bits that's why I just check them first. Checking the method is > > more expensive, so I defer it until I detect the upgrade header. > > > > > >> > >> 4. In this code: > >> > >> } else { /* websocket connection */ > >> s->current.server->keep_alive = HTTP_NO_KEEPALIVE; > >> s->client_info.keep_alive = HTTP_NO_KEEPALIVE; > >> heads->value_set(MIME_FIELD_CONNECTION, MIME_LEN_CONNECTION, > >> MIME_FIELD_UPGRADE, MIME_LEN_UPGRADE); > >> > >> if (s->is_websocket) { > >> heads->value_set(MIME_FIELD_UPGRADE, MIME_LEN_UPGRADE, "websocket", > >> 9); > >> } > >> > >> > >> > >> It seems it already thinks it is in "web socket" mode (the } else { > ...), > >> but in that else clause, it's checking for s->is_websocket(). This seems > >> odd, if anything, should it not be e.g. > >> > >> } else if (s->is_websocket) { > >> s->current.server->keep_alive = HTTP_NO_KEEPALIVE; > >> s->client_info.keep_alive = HTTP_NO_KEEPALIVE; > >> heads->value_set(MIME_FIELD_CONNECTION, MIME_LEN_CONNECTION, > >> MIME_FIELD_UPGRADE, MIME_LEN_UPGRADE); > >> heads->value_set(MIME_FIELD_UPGRADE, MIME_LEN_UPGRADE, "websocket", > 9); > >> } else { > >> // Some unhandled upgrade but not web socket path ? > >> } > >> > >> ? But maybe it's the comment that's wrong, and the } else { is > generically > >> for all upgrades, and within it, you have a small case for web socket? > >> Either way, it reads confusingly. > > > > I was trying to come up with a general pattern that could be easily > > extended if other protocols that require upgrade support are added. The > > idea is that all upgrade requests will need to set the Connection: > upgrade > > header, and only websocket connections will get the Upgrade: websocket > > header, it's more for readability for anyone adding more upgrade support > > later. It's the comment that makes it confusing, it should read } else { > /* > > upgrade case */, and then the inner if will check which protocol is being > > upgrade to. > > > > > >> > >> 5. This is mostly a question: Is there, or will there be, something such > >> that a plugin can "intercept" in the WebSocket tunnels? For example, I > can > >> imagine someone wanting to handle certain (but not all!) WebSocket > messages > >> in a plugin, e.g. fetching something out of cache. Saying "Future > >> enhancement" is good enough for me :). > > I believe transformations should already work out of the gate (although I > > haven't tried it). You should be able to create a request transformation > > and a response transformation to handle each side of the connection. I'm > > curious, so I'll try it. > > > > Also, to address your throttling question, the normal request flow isn't > > modified until _after_ the response comes back from the origin, so all > > normal things such as throttling should just work without issues, > because I > > took that approach that's why the patch is so small. > > > > > >> > >> Thanks! Hugely +1 on getting this in, it seems it's safe for non-WS > stuff, > >> and the code is surprisingly small (and clean). > >> > >> -- Leif > >> > >> >
