See responses below. On Fri, Jan 31, 2014 at 4:05 PM, Leif Hedstrom <zw...@apache.org> wrote:
> > On Jan 29, 2014, at 6:22 PM, Brian Geffon <bri...@apache.org> 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 > >
