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

[pkarc]

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

#387

[breck7]

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

+1

[indexzero]

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

[pkarc]

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

[glasser]

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]

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]

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 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]

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

[indexzero]

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]

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]

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

[breck7]

+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 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]

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

[breck7]

Awesome, thanks @indexzero , @cronopio !