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. 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. 3. In HttpTransact::handle_upgrade_request() why not check for the GET method and HTTP/1.1 at the top (“quickest way” …) ? 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. 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 :). 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
