http-proxy doesn't proxy the latest websocket specifications #97

Closed
3rd-Eden opened this Issue Aug 30, 2011 · 33 comments

Comments

Projects
None yet
8 participants
@3rd-Eden
Member

3rd-Eden commented Aug 30, 2011

Howdy,

A few days ago, we added FF6 and Chrome 14 websocket support to Socket.IO. But http-proxy doesn't seem to be able to proxy these upgrade events.

If I run a plain Socket.IO 0.8.2 chat example it works fine in FF6 and latest chrome, but if I add node-http-proxy infront of it only the Draft 76 specification receives a upgrade event.

After a bit digging I found out that https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L862 doesn't get fired for the new websocket specifications (it does work for older websocket specifications like draft 76)

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Aug 30, 2011

Member

@3rd-Eden. The agent object on the line that you've cited is an instance of the http.Agent from node.js core. So if the upgrade event is not firing on it this seems like a node.js core bug.

The new version of the websocket draft is on our list of priorities, but we probably won't implement it until the 0.6.x timeframe.

Member

indexzero commented Aug 30, 2011

@3rd-Eden. The agent object on the line that you've cited is an instance of the http.Agent from node.js core. So if the upgrade event is not firing on it this seems like a node.js core bug.

The new version of the websocket draft is on our list of priorities, but we probably won't implement it until the 0.6.x timeframe.

@3rd-Eden

This comment has been minimized.

Show comment
Hide comment
@3rd-Eden

3rd-Eden Aug 30, 2011

Member

@indexzero I don't know if it's a core bug, I don't know enough of the node-http-proxy module to debug it further. I might have missed something obvious.

Member

3rd-Eden commented Aug 30, 2011

@indexzero I don't know if it's a core bug, I don't know enough of the node-http-proxy module to debug it further. I might have missed something obvious.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Aug 30, 2011

Member

@3rd-Eden I'll try to take a stab at it if I get some cycles this week. NodeConf SummerCamp is next week so I'm kinda strapped for time.

Member

indexzero commented Aug 30, 2011

@3rd-Eden I'll try to take a stab at it if I get some cycles this week. NodeConf SummerCamp is next week so I'm kinda strapped for time.

@DTrejo

This comment has been minimized.

Show comment
Hide comment
@DTrejo

DTrejo Aug 30, 2011

+1

This issue makes me T_T
Having my no.de work faster will make me :D

Thanks guys!

DTrejo commented Aug 30, 2011

+1

This issue makes me T_T
Having my no.de work faster will make me :D

Thanks guys!

@nicokaiser

This comment has been minimized.

Show comment
Hide comment
@nicokaiser

nicokaiser Sep 9, 2011

Any news on this? I'd love to use node-http-proxy, but I need WebSocket (including newer drafts) support...

Any news on this? I'd love to use node-http-proxy, but I need WebSocket (including newer drafts) support...

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 9, 2011

Member

No. This is a difficult problem.

Member

indexzero commented Sep 9, 2011

No. This is a difficult problem.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 10, 2011

Member

@3rd-Eden I can confirm this. I can also confirm that Firefox 6 is not working either apparently. I did some digging and here's what I'm seeing

Firefox 7

{ host: 'localhost',
  port: 3000,
  method: 'GET',
  path: '/socket.io/1/websocket/1994801303780560091',
  headers: 
   { host: 'localhost:8000',
     'user-agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0.2) Gecko/20100101 Firefox/6.0.2',
     accept: 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
     'accept-language': 'en-us,en;q=0.5',
     'accept-encoding': 'gzip, deflate',
     'accept-charset': 'ISO-8859-1,utf-8;q=0.7,*;q=0.7',
     connection: 'keep-alive, Upgrade',
     'sec-websocket-version': '7',
     'sec-websocket-origin': 'http://localhost:8000',
     'sec-websocket-key': 'i6qqGU2E/s16IHmVLxhXEg==',
     pragma: 'no-cache',
     'cache-control': 'no-cache',
     upgrade: 'websocket' } }

Chrome 13

{ host: 'localhost',
  port: 3000,
  method: 'GET',
  path: '/socket.io/1/websocket/13704292091528906386',
  headers: 
   { upgrade: 'WebSocket',
     connection: 'Upgrade',
     host: 'localhost:8000',
     origin: 'http://localhost:8000',
     'sec-websocket-key1': '2 [ 1O09 k^ hr 0u.0123 0',
     'sec-websocket-key2': '31 0g[| 38.5DyvA6 9 4yqG  0U',
     cookie: '__utma=1.484068460.1314426697.1314426697.1314426697.1; __utmz=1.1314426697.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); _chartbeat2=xjfrmfu3529rbg7z' } }
Member

indexzero commented Sep 10, 2011

@3rd-Eden I can confirm this. I can also confirm that Firefox 6 is not working either apparently. I did some digging and here's what I'm seeing

Firefox 7

{ host: 'localhost',
  port: 3000,
  method: 'GET',
  path: '/socket.io/1/websocket/1994801303780560091',
  headers: 
   { host: 'localhost:8000',
     'user-agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0.2) Gecko/20100101 Firefox/6.0.2',
     accept: 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
     'accept-language': 'en-us,en;q=0.5',
     'accept-encoding': 'gzip, deflate',
     'accept-charset': 'ISO-8859-1,utf-8;q=0.7,*;q=0.7',
     connection: 'keep-alive, Upgrade',
     'sec-websocket-version': '7',
     'sec-websocket-origin': 'http://localhost:8000',
     'sec-websocket-key': 'i6qqGU2E/s16IHmVLxhXEg==',
     pragma: 'no-cache',
     'cache-control': 'no-cache',
     upgrade: 'websocket' } }

Chrome 13

{ host: 'localhost',
  port: 3000,
  method: 'GET',
  path: '/socket.io/1/websocket/13704292091528906386',
  headers: 
   { upgrade: 'WebSocket',
     connection: 'Upgrade',
     host: 'localhost:8000',
     origin: 'http://localhost:8000',
     'sec-websocket-key1': '2 [ 1O09 k^ hr 0u.0123 0',
     'sec-websocket-key2': '31 0g[| 38.5DyvA6 9 4yqG  0U',
     cookie: '__utma=1.484068460.1314426697.1314426697.1314426697.1; __utmz=1.1314426697.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); _chartbeat2=xjfrmfu3529rbg7z' } }
@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 10, 2011

Member

Clearly this isn't working in Firefox 6 because Firefox 6 implements WebSockets draft-07 (source: http://en.wikipedia.org/wiki/WebSocket)

Member

indexzero commented Sep 10, 2011

Clearly this isn't working in Firefox 6 because Firefox 6 implements WebSockets draft-07 (source: http://en.wikipedia.org/wiki/WebSocket)

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 10, 2011

Member

@3rd-Eden Did further investigation here. This may be a bug in node.js core (or more specifically the response parsing in http-parser so I'm going to attempt to summon @ry.

I can confirm deep in the tendrils of node (https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1230-1265), the upgrade event is not being fired for outgoing HTTP requests in the draft-07 protocol.

I'm no expert of the HttpParser used by node.js here, but it seems like the request and response parsing may be different code paths. Clearly the parser is in a different state:

"Request" mode (working): https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1020
"Response" mode (no working): https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1203

I'm building v0.5.5 now to see if this is fixed in HEAD.

Member

indexzero commented Sep 10, 2011

@3rd-Eden Did further investigation here. This may be a bug in node.js core (or more specifically the response parsing in http-parser so I'm going to attempt to summon @ry.

I can confirm deep in the tendrils of node (https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1230-1265), the upgrade event is not being fired for outgoing HTTP requests in the draft-07 protocol.

I'm no expert of the HttpParser used by node.js here, but it seems like the request and response parsing may be different code paths. Clearly the parser is in a different state:

"Request" mode (working): https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1020
"Response" mode (no working): https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1203

I'm building v0.5.5 now to see if this is fixed in HEAD.

@3rd-Eden

This comment has been minimized.

Show comment
Hide comment
@3rd-Eden

3rd-Eden Sep 10, 2011

Member

But the odd thing is that If we attach socket.io to a normal HTTP server it does work. So it's not a fundamental flaw on the node core, it might just be related to http client

On 10 sep. 2011, at 12:08, Charlie Robbins wrote:

@3rd-Eden Did further investigation here. This may be a bug in node.js core (or more specifically the response parsing in http-parser so I'm going to attempt to summon @ry.

I can confirm deep in the tendrils of node (https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1230-1265), the upgrade event is not being fired for outgoing HTTP requests in the draft-07 protocol.

I'm no expert of the HttpParser used by node.js here, but it seems like the request and response parsing may be different code paths. Clearly the parser is in a different state:

"Request" mode (working): https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1020
"Response" mode (no working): https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1203

I'm building v0.5.5 now to see if this is fixed in HEAD.

Reply to this email directly or view it on GitHub:
#97 (comment)

Member

3rd-Eden commented Sep 10, 2011

But the odd thing is that If we attach socket.io to a normal HTTP server it does work. So it's not a fundamental flaw on the node core, it might just be related to http client

On 10 sep. 2011, at 12:08, Charlie Robbins wrote:

@3rd-Eden Did further investigation here. This may be a bug in node.js core (or more specifically the response parsing in http-parser so I'm going to attempt to summon @ry.

I can confirm deep in the tendrils of node (https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1230-1265), the upgrade event is not being fired for outgoing HTTP requests in the draft-07 protocol.

I'm no expert of the HttpParser used by node.js here, but it seems like the request and response parsing may be different code paths. Clearly the parser is in a different state:

"Request" mode (working): https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1020
"Response" mode (no working): https://github.com/joyent/node/blob/v0.4.11/lib/http.js#L1203

I'm building v0.5.5 now to see if this is fixed in HEAD.

Reply to this email directly or view it on GitHub:
#97 (comment)

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 10, 2011

Member

@3rd-Eden @ry I can confirm that this still isn't working in Firefox 6 and Chrome 14 upon upgrading to http2 from @mikeal in v0.5.5.

I would like to make note for @mikeal that http2 is going to simplify the implementation quite a bit because the upgrade event is fired on the ClientRequest object and not the Agent.

However, the upgrade event is still not firing and I'm starting to think that this is a problem with http-parser since it seems to hold the necessary and sufficient state for the event to be fired: https://github.com/joyent/node/blob/v0.5.5/lib/http2.js#L1107

@ry @mikeal Any thoughts here? I'm stumped.

Member

indexzero commented Sep 10, 2011

@3rd-Eden @ry I can confirm that this still isn't working in Firefox 6 and Chrome 14 upon upgrading to http2 from @mikeal in v0.5.5.

I would like to make note for @mikeal that http2 is going to simplify the implementation quite a bit because the upgrade event is fired on the ClientRequest object and not the Agent.

However, the upgrade event is still not firing and I'm starting to think that this is a problem with http-parser since it seems to hold the necessary and sufficient state for the event to be fired: https://github.com/joyent/node/blob/v0.5.5/lib/http2.js#L1107

@ry @mikeal Any thoughts here? I'm stumped.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 10, 2011

Member

The http2 compatible of node-http-proxy is here: https://github.com/nodejitsu/node-http-proxy/tree/http2

Member

indexzero commented Sep 10, 2011

The http2 compatible of node-http-proxy is here: https://github.com/nodejitsu/node-http-proxy/tree/http2

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 10, 2011

Member

@3rd-Eden

But the odd thing is that If we attach socket.io to a normal HTTP server it does work. So it's not a fundamental flaw on
the node core, it might just be related to http client

Exactly. When you attach socket.io to a normal HTTP server the http-parser is in the "Request" state. When node-http-proxy is attempting to make an outgoing WebSocket connection it is in the "Response" state. I noticed that the only current implementation of the later spec(s) in node is actually using the deprecated http.createClient method because they claim "Node's new Agent-based API is buggy."

I'm not sure if these bugs are in the http-parser or the Agent but as I tried this out with both http.js and http2.js only to get the same result I'm suspicious of it being at the http-level. No evidence to support that claim though.

https://github.com/Worlize/WebSocket-Node/blob/master/lib/WebSocketClient.js#L149-150

Member

indexzero commented Sep 10, 2011

@3rd-Eden

But the odd thing is that If we attach socket.io to a normal HTTP server it does work. So it's not a fundamental flaw on
the node core, it might just be related to http client

Exactly. When you attach socket.io to a normal HTTP server the http-parser is in the "Request" state. When node-http-proxy is attempting to make an outgoing WebSocket connection it is in the "Response" state. I noticed that the only current implementation of the later spec(s) in node is actually using the deprecated http.createClient method because they claim "Node's new Agent-based API is buggy."

I'm not sure if these bugs are in the http-parser or the Agent but as I tried this out with both http.js and http2.js only to get the same result I'm suspicious of it being at the http-level. No evidence to support that claim though.

https://github.com/Worlize/WebSocket-Node/blob/master/lib/WebSocketClient.js#L149-150

@3rd-Eden

This comment has been minimized.

Show comment
Hide comment
@3rd-Eden

3rd-Eden Sep 10, 2011

Member

So if http-proxy starts using the old interface this would be resolved i guess?

On 10 sep. 2011, at 13:16, Charlie Robbins wrote:

@3rd-Eden

But the odd thing is that If we attach socket.io to a normal HTTP server it does work. So it's not a fundamental flaw on
the node core, it might just be related to http client

Exactly. When you attach socket.io to a normal HTTP server the http-parser is in the "Request" state. When node-http-proxy is attempting to make an outgoing WebSocket connection it is in the "Response" state. I noticed that the only current implementation of the later spec(s) in node is actually using the deprecated http.createClient method because they claim "Node's new Agent-based API is buggy."

https://github.com/Worlize/WebSocket-Node/blob/master/lib/WebSocketClient.js#L149-150

Reply to this email directly or view it on GitHub:
#97 (comment)

Member

3rd-Eden commented Sep 10, 2011

So if http-proxy starts using the old interface this would be resolved i guess?

On 10 sep. 2011, at 13:16, Charlie Robbins wrote:

@3rd-Eden

But the odd thing is that If we attach socket.io to a normal HTTP server it does work. So it's not a fundamental flaw on
the node core, it might just be related to http client

Exactly. When you attach socket.io to a normal HTTP server the http-parser is in the "Request" state. When node-http-proxy is attempting to make an outgoing WebSocket connection it is in the "Response" state. I noticed that the only current implementation of the later spec(s) in node is actually using the deprecated http.createClient method because they claim "Node's new Agent-based API is buggy."

https://github.com/Worlize/WebSocket-Node/blob/master/lib/WebSocketClient.js#L149-150

Reply to this email directly or view it on GitHub:
#97 (comment)

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 10, 2011

Member

Thats not a viable solution

Member

indexzero commented Sep 10, 2011

Thats not a viable solution

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 11, 2011

Member

@3rd-Eden Wanted to give more info on my last comment. It was like 8am here; so I was pretty spent after pushing out 0.7.0.

From what I can see in http2 using the deprecated http.Client interface will not work because it is using the new ClientRequest object which is not raising the upgrade event. I'll try to do some more digging, but this seems like a very small header parsing issue.

Member

indexzero commented Sep 11, 2011

@3rd-Eden Wanted to give more info on my last comment. It was like 8am here; so I was pretty spent after pushing out 0.7.0.

From what I can see in http2 using the deprecated http.Client interface will not work because it is using the new ClientRequest object which is not raising the upgrade event. I'll try to do some more digging, but this seems like a very small header parsing issue.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 11, 2011

Member

@3rd-Eden @ry

I spent some time reading the http-parser code with help from @bmeck and it seems that this is indeed a problem with the connection header parsing not detecting the "Upgrade" state because in Firefox 7 it sends

connection: keep-alive, Upgrade

where as in Chrome 13 and older versions of the spec the header is:

connection: Upgrade

Based on our first pass it seems like the parser enters the h_connection_keep_alive greedily (https://github.com/ry/http-parser/blob/master/http_parser.c#L1367-1377) and does not continue to check the contents of the header.

Not sure how simple a fix this is, but @bmeck will be putting together a low-level parser repro next week.

Member

indexzero commented Sep 11, 2011

@3rd-Eden @ry

I spent some time reading the http-parser code with help from @bmeck and it seems that this is indeed a problem with the connection header parsing not detecting the "Upgrade" state because in Firefox 7 it sends

connection: keep-alive, Upgrade

where as in Chrome 13 and older versions of the spec the header is:

connection: Upgrade

Based on our first pass it seems like the parser enters the h_connection_keep_alive greedily (https://github.com/ry/http-parser/blob/master/http_parser.c#L1367-1377) and does not continue to check the contents of the header.

Not sure how simple a fix this is, but @bmeck will be putting together a low-level parser repro next week.

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Sep 11, 2011

please provide an example request that youve seen in the wild with connection: keep-alive, Upgrade and i'll fix the parser

ry commented Sep 11, 2011

please provide an example request that youve seen in the wild with connection: keep-alive, Upgrade and i'll fix the parser

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 11, 2011

Member

@ry Firefox 7 Websocket Requests. Seems to be consistent across Firefox 6 (draft7), Firefox 7 (draft10) and (reportedly) Chrome 14 (draft10).

More info in this comment: #97 (comment)

Member

indexzero commented Sep 11, 2011

@ry Firefox 7 Websocket Requests. Seems to be consistent across Firefox 6 (draft7), Firefox 7 (draft10) and (reportedly) Chrome 14 (draft10).

More info in this comment: #97 (comment)

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 11, 2011

Member

Best way to repoduce this is in the wild is to open the simple chat example for socket.io that @3rd-Eden suggested in Firefox 6 or 7. I'm using a different socket.io sample app: http://github.com/fat/space-tweet. To run this just

  [Update config.json with valid Twitter credentials]
  npm install
  node server.js

Then you can throw a proxy server in front of it

  require('http-proxy').createServer(3000, 'localhost').listen(8000);

I found an interesting discussion on the ietf mailing list about a bug in the draft11 spec which confirms that comma-separated connection headers are in-fact supported by HTTP and used in builds of Firefox: http://www.ietf.org/mail-archive/web/hybi/current/msg08519.html

"Connection" header allows a list of tokens separated by comma. So the
following is valid (in fact Firefox 8 uses it):

Connection: keep-alive, upgrade

Member

indexzero commented Sep 11, 2011

Best way to repoduce this is in the wild is to open the simple chat example for socket.io that @3rd-Eden suggested in Firefox 6 or 7. I'm using a different socket.io sample app: http://github.com/fat/space-tweet. To run this just

  [Update config.json with valid Twitter credentials]
  npm install
  node server.js

Then you can throw a proxy server in front of it

  require('http-proxy').createServer(3000, 'localhost').listen(8000);

I found an interesting discussion on the ietf mailing list about a bug in the draft11 spec which confirms that comma-separated connection headers are in-fact supported by HTTP and used in builds of Firefox: http://www.ietf.org/mail-archive/web/hybi/current/msg08519.html

"Connection" header allows a list of tokens separated by comma. So the
following is valid (in fact Firefox 8 uses it):

Connection: keep-alive, upgrade

@3rd-Eden

This comment has been minimized.

Show comment
Hide comment
@3rd-Eden

3rd-Eden Sep 11, 2011

Member

Well there seems to be 2 seperate issues then. Because Chrome beta and Chromium are sending the following headers:

Connection:Upgrade
Host:observer.no.de
Sec-WebSocket-Key:l18CXsxK2DurU2awtNQwKg==
Sec-WebSocket-Origin:http://observer.no.de
Sec-WebSocket-Version:8
Upgrade:websocket
(Key3):00:00:00:00:00:00:00:00

And they are not answered correctly by the proxy either. So it seems to me that the

connection: keep-alive, Upgrade

Isn't the only reason why node-http-proxy fails to proxy requests.

Member

3rd-Eden commented Sep 11, 2011

Well there seems to be 2 seperate issues then. Because Chrome beta and Chromium are sending the following headers:

Connection:Upgrade
Host:observer.no.de
Sec-WebSocket-Key:l18CXsxK2DurU2awtNQwKg==
Sec-WebSocket-Origin:http://observer.no.de
Sec-WebSocket-Version:8
Upgrade:websocket
(Key3):00:00:00:00:00:00:00:00

And they are not answered correctly by the proxy either. So it seems to me that the

connection: keep-alive, Upgrade

Isn't the only reason why node-http-proxy fails to proxy requests.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 11, 2011

Member

Interesting. Again, in this case as you pointed out @3rd-Eden it is just that the upgrade event is not being emitted. This leads me to believe that this is not a bug in node-http-proxy, but perhaps a combination of small issues elsewhere in node.js core or it's dependencies.

Member

indexzero commented Sep 11, 2011

Interesting. Again, in this case as you pointed out @3rd-Eden it is just that the upgrade event is not being emitted. This leads me to believe that this is not a bug in node-http-proxy, but perhaps a combination of small issues elsewhere in node.js core or it's dependencies.

@narup

This comment has been minimized.

Show comment
Hide comment
@narup

narup Sep 17, 2011

I wonder where is (Key3):00:00:00:00:00:00:00:00 coming from, I am trying to implement draft-ietf-hybi-thewebsocketprotocol-15 in jwebsocket.org. But keep getting disconnected. I am using chrome 14.0.835.163

narup commented Sep 17, 2011

I wonder where is (Key3):00:00:00:00:00:00:00:00 coming from, I am trying to implement draft-ietf-hybi-thewebsocketprotocol-15 in jwebsocket.org. But keep getting disconnected. I am using chrome 14.0.835.163

@mmalecki

This comment has been minimized.

Show comment
Hide comment
@mmalecki

mmalecki Sep 21, 2011

Contributor

I think I might know what causes this bug.

Websockets are messed up a bit in Chrome 14 (I remember that this was a real issue during node knockout), and Firefox's support for them is limited. As you can see, neither browser supports binary frames, while socket.io seems to be using it.

In fact, I was able to run socket.io chat demo with node-http-proxy and Chrome 15.0.874.15 dev (and yes, it was using Websockets transport - verified by socket.socket.transport in inspector and by looking at chrome://net-internals/#events).
Firefox 8.0a works partially. Connection breaks quite randomly, but that happens both with and without proxy, so I guess it isn't our problem.
Also, Chromium 13 works without any problems.

For me, it seems that the source of problem is that browsers are trying to use newest hybi specification, but don't actually implement it fully.

However, please note that I may be terribly wrong and YMMV.

Contributor

mmalecki commented Sep 21, 2011

I think I might know what causes this bug.

Websockets are messed up a bit in Chrome 14 (I remember that this was a real issue during node knockout), and Firefox's support for them is limited. As you can see, neither browser supports binary frames, while socket.io seems to be using it.

In fact, I was able to run socket.io chat demo with node-http-proxy and Chrome 15.0.874.15 dev (and yes, it was using Websockets transport - verified by socket.socket.transport in inspector and by looking at chrome://net-internals/#events).
Firefox 8.0a works partially. Connection breaks quite randomly, but that happens both with and without proxy, so I guess it isn't our problem.
Also, Chromium 13 works without any problems.

For me, it seems that the source of problem is that browsers are trying to use newest hybi specification, but don't actually implement it fully.

However, please note that I may be terribly wrong and YMMV.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 21, 2011

Member

@mmalecki Thanks for the information, but @3rdeden has confirmed that socket.io@latest works on node stand-alone, but not behind node-http-proxy.

So the binary frames may be a red herring, but I will look into it.

Member

indexzero commented Sep 21, 2011

@mmalecki Thanks for the information, but @3rdeden has confirmed that socket.io@latest works on node stand-alone, but not behind node-http-proxy.

So the binary frames may be a red herring, but I will look into it.

@mmalecki

This comment has been minimized.

Show comment
Hide comment
@mmalecki

mmalecki Sep 29, 2011

Contributor

After some further investigation with @AvianFlu:

Parser doesn't seem to be a problem. It detects upgrades just fine (and fires this callback). Proxy receives http requests like:

GET /socket.io/1/websocket/8800604461110870103 HTTP/1.1
Upgrade: WebSocket
Connection: Upgrade
Host: localhost:9000
Origin: http://localhost:9000
Sec-WebSocket-Key1: 4 00 q 9g1  94    83 0
Sec-WebSocket-Key2: &H 15453t 16A616

But fails to pass them further. This callback doesn't get executed at all.

Comparison between direct and proxied request - forwarded requests have their headers all in lower case. No forwarded request has an Upgrade header.

I'll investigate it further, but it's 4 AM and it's basically a brain dump.

Contributor

mmalecki commented Sep 29, 2011

After some further investigation with @AvianFlu:

Parser doesn't seem to be a problem. It detects upgrades just fine (and fires this callback). Proxy receives http requests like:

GET /socket.io/1/websocket/8800604461110870103 HTTP/1.1
Upgrade: WebSocket
Connection: Upgrade
Host: localhost:9000
Origin: http://localhost:9000
Sec-WebSocket-Key1: 4 00 q 9g1  94    83 0
Sec-WebSocket-Key2: &H 15453t 16A616

But fails to pass them further. This callback doesn't get executed at all.

Comparison between direct and proxied request - forwarded requests have their headers all in lower case. No forwarded request has an Upgrade header.

I'll investigate it further, but it's 4 AM and it's basically a brain dump.

@mmalecki

This comment has been minimized.

Show comment
Hide comment
@mmalecki

mmalecki Sep 29, 2011

Contributor

After some further further investigation:

In this line we append a message to a queue and we hope that eventually (when socket is assigned) it'll be send to a server. In fact, socket gets assigned in the very first _cycle call. This calls a _flush method on this message, but flushing fails here, because of empty output and this request gets stuck.

Weirdest thing is: after some time proxyError gets called with 'socket hung up' message. And then this OutgoingMessage, which got stuck is send to a server (to be precise: OutgoingMessage._writeRaw is called - of course, server never receives it because of this socket error).

In fact, I can force this flush by inserting first.end() right after this line or reverseProxy.end() here, but it results in a following error from socket.io:

   debug - websocket writing 1::
connected
   warn  - websocket parser error: reserved fields not empty
   info  - transport end
   debug - set close timeout for client 6597731141049970547
   debug - cleared close timeout for client 6597731141049970547
   debug - cleared heartbeat interval for client 6597731141049970547
   debug - discarding transport
   warn  - websocket parser error: no handler for opcode 15
Contributor

mmalecki commented Sep 29, 2011

After some further further investigation:

In this line we append a message to a queue and we hope that eventually (when socket is assigned) it'll be send to a server. In fact, socket gets assigned in the very first _cycle call. This calls a _flush method on this message, but flushing fails here, because of empty output and this request gets stuck.

Weirdest thing is: after some time proxyError gets called with 'socket hung up' message. And then this OutgoingMessage, which got stuck is send to a server (to be precise: OutgoingMessage._writeRaw is called - of course, server never receives it because of this socket error).

In fact, I can force this flush by inserting first.end() right after this line or reverseProxy.end() here, but it results in a following error from socket.io:

   debug - websocket writing 1::
connected
   warn  - websocket parser error: reserved fields not empty
   info  - transport end
   debug - set close timeout for client 6597731141049970547
   debug - cleared close timeout for client 6597731141049970547
   debug - cleared heartbeat interval for client 6597731141049970547
   debug - discarding transport
   warn  - websocket parser error: no handler for opcode 15
@AvianFlu

This comment has been minimized.

Show comment
Hide comment
@AvianFlu

AvianFlu Sep 29, 2011

Contributor

The request gets stuck, as @mmalecki said, and then, since there's no error status, eventually https://github.com/joyent/node/blob/v0.4.12/lib/http.js#L1284-1293 is reached and the "socket hang up" message is received. The real key to fixing this lies in figuring out how to make it past https://github.com/joyent/node/blob/v0.4.12/lib/http.js#L753 to the socket.write() call at the end of that method.

Contributor

AvianFlu commented Sep 29, 2011

The request gets stuck, as @mmalecki said, and then, since there's no error status, eventually https://github.com/joyent/node/blob/v0.4.12/lib/http.js#L1284-1293 is reached and the "socket hang up" message is received. The real key to fixing this lies in figuring out how to make it past https://github.com/joyent/node/blob/v0.4.12/lib/http.js#L753 to the socket.write() call at the end of that method.

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Sep 29, 2011

Do you guys have a simple test case yet? Put one together send it to me and I'll just fix it. You guys are thinking too hard.

A simple test case would be: tcpdump the req/res in question, set up a http server, have a tcp client connect to it - send the request as you got it from the tcpdump - assert what you expect to happen. See test/simple/test-http-* in the node tree for examples of test cases.

ry commented Sep 29, 2011

Do you guys have a simple test case yet? Put one together send it to me and I'll just fix it. You guys are thinking too hard.

A simple test case would be: tcpdump the req/res in question, set up a http server, have a tcp client connect to it - send the request as you got it from the tcpdump - assert what you expect to happen. See test/simple/test-http-* in the node tree for examples of test cases.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 29, 2011

Member

@ry There really isn't a fully programmatic test case. Is that what you mean by a "simple test case". No node.js library (node-websocket-client, websocket-node, etc) correctly emulates how browsers have implemented WebSockets.

The easiest way to reproduce this right now is to

  1. Write anything that uses socket.io (we have some simple test cases if you'd like to talk offline)
  2. Put node-http-proxy in front of them
  3. Open it in Firefox 7+ or Chrome 14+
Member

indexzero commented Sep 29, 2011

@ry There really isn't a fully programmatic test case. Is that what you mean by a "simple test case". No node.js library (node-websocket-client, websocket-node, etc) correctly emulates how browsers have implemented WebSockets.

The easiest way to reproduce this right now is to

  1. Write anything that uses socket.io (we have some simple test cases if you'd like to talk offline)
  2. Put node-http-proxy in front of them
  3. Open it in Firefox 7+ or Chrome 14+
@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 30, 2011

Member

I can confirm that the fix from @indutny resolves this issue in his pull request. In both Firefox 7 and Chrome 14. I'm merging this into master.

Member

indexzero commented Sep 30, 2011

I can confirm that the fix from @indutny resolves this issue in his pull request. In both Firefox 7 and Chrome 14. I'm merging this into master.

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Sep 30, 2011

Member

This should be fixed in 0.7.2. @3rd-Eden can you confirm?

Member

indexzero commented Sep 30, 2011

This should be fixed in 0.7.2. @3rd-Eden can you confirm?

@indexzero indexzero closed this Sep 30, 2011

@3rd-Eden

This comment has been minimized.

Show comment
Hide comment
@3rd-Eden

3rd-Eden Oct 1, 2011

Member

@indexzero fix confirmed

Member

3rd-Eden commented Oct 1, 2011

@indexzero fix confirmed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment