Fix for issue #387 websocket proxy not work in node.js version 0.10.0 #402

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@pkarc
Contributor

pkarc commented Apr 8, 2013

The headers in the 'handshake' event were not written to the socket, the client received data but not the headers.

#387

Ivan Jaramillo
Add headers on 'handshake'
The headers in the 'handshake' event were not written to the socket, the client received data but not the headers.
@breck7

This comment has been minimized.

Show comment
Hide comment
@breck7

breck7 Apr 10, 2013

thank you @pkarc! just lost 5 hours on this one. works for us.

+1

breck7 commented Apr 10, 2013

thank you @pkarc! just lost 5 hours on this one. works for us.

+1

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Apr 10, 2013

Member

@pkarc Thanks for this. Can you confirm the fix is backwards compatible with node@0.8.x?

Member

indexzero commented Apr 10, 2013

@pkarc Thanks for this. Can you confirm the fix is backwards compatible with node@0.8.x?

@pkarc

This comment has been minimized.

Show comment
Hide comment
@pkarc

pkarc Apr 13, 2013

Contributor

Tested on v0.7.0, v0.8.0, v0.8.2 and v0.8.6, and it works!

Contributor

pkarc commented Apr 13, 2013

Tested on v0.7.0, v0.8.0, v0.8.2 and v0.8.6, and it works!

@glasser

This comment has been minimized.

Show comment
Hide comment
@glasser

glasser Apr 17, 2013

Contributor

This helps a bit (the headers aren't dropped like they are without this patch) but I'm still seeing weird results where the first data in the websocket gets written twice across the proxy.

Contributor

glasser commented Apr 17, 2013

This helps a bit (the headers aren't dropped like they are without this patch) but I'm still seeing weird results where the first data in the websocket gets written twice across the proxy.

@glasser

This comment has been minimized.

Show comment
Hide comment
@glasser

glasser Apr 17, 2013

Contributor

Specifically, it looks like the listeners.onIncoming is set too early. This is an on('data') on the same socket as the handshake on('data'), and it looks like the same bit of data can get sent to both and doubly-proxied to the client.

Contributor

glasser commented Apr 17, 2013

Specifically, it looks like the listeners.onIncoming is set too early. This is an on('data') on the same socket as the handshake on('data'), and it looks like the same bit of data can get sent to both and doubly-proxied to the client.

@glasser

This comment has been minimized.

Show comment
Hide comment
@glasser

glasser Apr 17, 2013

Contributor

Yeah. Trace through the logic.

In http.js ClientRequest.prorotype.onSocket, we set socket.ondata = socketOnData and immediately emit a socket event. This triggers the reverseProxy.once('socket') which sets the handshake on('data'). This also switches the socket's stream into 0.8 emulation mode.

Then we actually read the headers over the network. By my reading of onread in net.js, this calls socket.ondata directly rather than invoking the stream API (that's the self.push there). This is the socketOnData in http.js, which parses the headers and decides it's an upgrade. First it clears socket.ondata (switching the socket into "use the normal stream API" mode) and then it invokes the 'upgrade' handler on reverseProxy.

This in turn (with this patch) saves the headers into reverseProxy.handshake and calls our onUpgrade function, which among other things sets the listeners.onIncoming listener on the socket. Now we have two separate data listeners on the socket... and we still haven't invoked either!

(BTW, if the initial block of data sent to socketOnData contained some bytes past the headers, this will be lost! This is the bodyHead in socketOnData, which is the ignored head argument in the reverseProxy.on('upgrade') handler. But this isn't the issue I'm seeing...)

Finally, the proxied server sends the beginning of its body. This goes BOTH to handshake and to listeners.onIncoming, and the first part of the body gets double-printed!

Seems to me that the contents of handshake should be done as a first-time-only step inside listeners.onIncoming instead of settings up two separate listeners that may overlap.

Contributor

glasser commented Apr 17, 2013

Yeah. Trace through the logic.

In http.js ClientRequest.prorotype.onSocket, we set socket.ondata = socketOnData and immediately emit a socket event. This triggers the reverseProxy.once('socket') which sets the handshake on('data'). This also switches the socket's stream into 0.8 emulation mode.

Then we actually read the headers over the network. By my reading of onread in net.js, this calls socket.ondata directly rather than invoking the stream API (that's the self.push there). This is the socketOnData in http.js, which parses the headers and decides it's an upgrade. First it clears socket.ondata (switching the socket into "use the normal stream API" mode) and then it invokes the 'upgrade' handler on reverseProxy.

This in turn (with this patch) saves the headers into reverseProxy.handshake and calls our onUpgrade function, which among other things sets the listeners.onIncoming listener on the socket. Now we have two separate data listeners on the socket... and we still haven't invoked either!

(BTW, if the initial block of data sent to socketOnData contained some bytes past the headers, this will be lost! This is the bodyHead in socketOnData, which is the ignored head argument in the reverseProxy.on('upgrade') handler. But this isn't the issue I'm seeing...)

Finally, the proxied server sends the beginning of its body. This goes BOTH to handshake and to listeners.onIncoming, and the first part of the body gets double-printed!

Seems to me that the contents of handshake should be done as a first-time-only step inside listeners.onIncoming instead of settings up two separate listeners that may overlap.

@breck7

This comment has been minimized.

Show comment
Hide comment
@breck7

breck7 Apr 17, 2013

This also is helping us a bit, but not quite a full fix--still getting weird behavior. Haven't quite dived down into the details as much as @glasser, but I can second that this is not quite a full fix.

breck7 commented Apr 17, 2013

This also is helping us a bit, but not quite a full fix--still getting weird behavior. Haven't quite dived down into the details as much as @glasser, but I can second that this is not quite a full fix.

@glasser

This comment has been minimized.

Show comment
Hide comment
@glasser

glasser Apr 17, 2013

Contributor

(Or, well, this whole thing probably should be rewritten to use Node 0.10-style readable events, but...)

Contributor

glasser commented Apr 17, 2013

(Or, well, this whole thing probably should be rewritten to use Node 0.10-style readable events, but...)

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Apr 18, 2013

Member

Because of streams2, node-http-proxy could use a pretty serious rewrite internally for many reasons. This patch will be merged as a stop-gap, and the next work will go into a 0.10+ only version.

Member

indexzero commented Apr 18, 2013

Because of streams2, node-http-proxy could use a pretty serious rewrite internally for many reasons. This patch will be merged as a stop-gap, and the next work will go into a 0.10+ only version.

@aalness

This comment has been minimized.

Show comment
Hide comment
@aalness

aalness May 9, 2013

Shouldn't it also proxy back the value of 'Sec-WebSocket-Protocol'?

aalness commented on ad57160 May 9, 2013

Shouldn't it also proxy back the value of 'Sec-WebSocket-Protocol'?

@aalness

This comment has been minimized.

Show comment
Hide comment
@aalness

aalness May 10, 2013

A third on this being insufficient:

This doesn't seem to work on node v0.8.7 w/SSL (not sure if SSL is significant to the problem.) Also in my application the server end (proxy target) begins sending data first, again, not sure if that matters.

I see the 'ondata' event for the socket fire first and call the 'handshake' function but meanwhile the 'upgrade' event hasn't fired yet to populate the status code and the 101 response headers so they are null and still don't find their way back to the browser to complete the handshake.

I should add my browser is Chrome 26.

aalness commented May 10, 2013

A third on this being insufficient:

This doesn't seem to work on node v0.8.7 w/SSL (not sure if SSL is significant to the problem.) Also in my application the server end (proxy target) begins sending data first, again, not sure if that matters.

I see the 'ondata' event for the socket fire first and call the 'handshake' function but meanwhile the 'upgrade' event hasn't fired yet to populate the status code and the 101 response headers so they are null and still don't find their way back to the browser to complete the handshake.

I should add my browser is Chrome 26.

@danhowitt

This comment has been minimized.

Show comment
Hide comment
@danhowitt

danhowitt Jul 19, 2013

Anyone looking at this? Seems node 0.10.13V isn't compatible with node-http-proxy - websockets when proxied are dropped.

Anyone looking at this? Seems node 0.10.13V isn't compatible with node-http-proxy - websockets when proxied are dropped.

@breck7

This comment has been minimized.

Show comment
Hide comment
@breck7

breck7 Jul 19, 2013

+1

Any way we can help short of leading a rewrite (I don't know the code well enough).

Works so great on 0.8.x and would love to be able to use node-http-proxy with 10.

breck7 commented Jul 19, 2013

+1

Any way we can help short of leading a rewrite (I don't know the code well enough).

Works so great on 0.8.x and would love to be able to use node-http-proxy with 10.

@aalness

This comment has been minimized.

Show comment
Hide comment
@aalness

aalness Jul 19, 2013

This diff looks terrible but it seems to work well for the current version of the WebSocket protocol. I haven't tested it with the older drafts. It probably doesn't work for them.

I don't have time to test it for older implementations but I'm using it in my project without issue.

Diff: http://www.onegrandcircle.com/~andy/ws.diff

aalness commented Jul 19, 2013

This diff looks terrible but it seems to work well for the current version of the WebSocket protocol. I haven't tested it with the older drafts. It probably doesn't work for them.

I don't have time to test it for older implementations but I'm using it in my project without issue.

Diff: http://www.onegrandcircle.com/~andy/ws.diff

@indexzero

This comment has been minimized.

Show comment
Hide comment
@indexzero

indexzero Jul 19, 2013

Member

We are working on a complete rewrite for streams2. See the work from @cronopio on the 0.10.x branch.

Member

indexzero commented Jul 19, 2013

We are working on a complete rewrite for streams2. See the work from @cronopio on the 0.10.x branch.

@breck7

This comment has been minimized.

Show comment
Hide comment
@breck7

breck7 Jul 20, 2013

Awesome, thanks @indexzero , @cronopio !

breck7 commented Jul 20, 2013

Awesome, thanks @indexzero , @cronopio !

@ewindso ewindso referenced this pull request in aldeed/deploymeteor Aug 27, 2013

Closed

websockets with multiple apps? #3

AlexeyMK pushed a commit to AlexeyMK/meteor that referenced this pull request Nov 13, 2013

Upgrade node-http-proxy to 0.10.1.
Intentionally not choosing 0.10.2, which has a websocket proxying Node 0.10
semi-fix which I found to sometimes corrupt data (on Node 0.10). See my analysis
on nodejitsu/node-http-proxy#402

I do not know whether or not the PR corrupts data on 0.8, but it isn't necessary
there, so I'm going to hold off on taking that change until the promised future
complete rewrite of http-proxy to use the new 0.10 streams API.

mitar pushed a commit to mitar/blaze-pruned that referenced this pull request Dec 5, 2016

Upgrade node-http-proxy to 0.10.1.
Intentionally not choosing 0.10.2, which has a websocket proxying Node 0.10
semi-fix which I found to sometimes corrupt data (on Node 0.10). See my analysis
on nodejitsu/node-http-proxy#402

I do not know whether or not the PR corrupts data on 0.8, but it isn't necessary
there, so I'm going to hold off on taking that change until the promised future
complete rewrite of http-proxy to use the new 0.10 streams API.

kenieevan pushed a commit to kenieevan/bsg that referenced this pull request Mar 16, 2017

Andy Alness
Fix WebSocket proxying in the ABSG.
This is primarily a re-working of:
nodejitsu/node-http-proxy#402

That patch doesn't seem to work completely, as noted.

Tested w/Chrome, Safari, and Firefox
Note: There is some flakiness with Firefox which needs
to be investigated but this patch is already a vast
improvement over the existing situation.

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