You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Leif Hedstrom <zw...@apache.org> on 2014/02/01 01:05:28 UTC
Re: WebSocket support
On Jan 29, 2014, at 6:22 PM, Brian Geffon <br...@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
Re: WebSocket support
Posted by Leif Hedstrom <zw...@apache.org>.
On Jan 31, 2014, at 5:05 PM, Leif Hedstrom <zw...@apache.org> wrote:
>
> On Jan 29, 2014, at 6:22 PM, Brian Geffon <br...@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.
Also, forgot to include it, but there is (I’m pretty sure) logic around counting client connections and using that to decide if a new connection should be accepted or not (“throttling”). This might affect that as well.
— leif
Re: WebSocket support
Posted by Brian Geffon <br...@apache.org>.
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 <briang@apache.org<javascript:;>>
> wrote:
> >
> > See responses below.
> >
> >> On Fri, Jan 31, 2014 at 4:05 PM, Leif Hedstrom <zwoop@apache.org<javascript:;>>
> wrote:
> >>
> >>
> >>> On Jan 29, 2014, at 6:22 PM, Brian Geffon <briang@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
> >>
> >>
>
Re: WebSocket support
Posted by Leif Hedstrom <zw...@apache.org>.
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 <br...@apache.org> wrote:
>
> 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 <br...@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
>>
>>
Re: WebSocket support
Posted by Brian Geffon <br...@apache.org>.
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 <br...@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
>
>