unusual behavior when proxying websocket to socket.io

[jfis]

I'm not really sure how to explain so
I whipped up some code to demonstrate.

https://gist.github.com/967964

please run that, then open 3 (console-supporting) browsers at http://localhost:8000
(i hope to find something to simulate in the future)
hit send in each
(you may need to modify node-http-proxy to not change headers)
console will log msgs rcvd from server.

then open 3 more browsers at http://localhost:9000 (no proxy)
hit send in each

notice the output is different.
also, the server output is different.

when proxied, messages are duplicated for client X depending on the number of clients connected after X.
also, connections disconnect after some time when proxied while they remain open indefinitely when not proxied.

node 0.4.7
npm 1.0.6
node-http-proxy 0.5.1
socket.io 0.6.17

[jfis]

The disconnects happen on the heartbeat timeout. Not sure what isn't passed through node-http-proxy yet.

The duplicate messages happen because onUpgrade() gets called every time any client connects which adds duplicate listeners. It seems agent (of _getAgent) is the same object for all connections. onUpgrade is invoked in a listener for the agent 'upgrade' event.

[jfis]

preventing duplicates by checking whether it already has listeners first seems to fix the disconnects as well. I guess heartbeats were being duplicated but only unique heartbeat numbers get processed. I'm not convinced what I've implemented is the proper solution though.

[indexzero]

Not exactly sure what's going on here, but I'll investigate over the weekend. Thanks.

[dazagrohovaz]

Hi Charlie,

i've tried to reproduce this issue, i got in the console "Error during WebSocket handshake: origin mismatch: http://localhost:8000 != http://localhost"

I guess you need to attach the port in both cases, at line 589 and 590, on your Lib: node-http-proxy.js
or use the remoteHost variable created at line 586.

line 586: remoteHost = options.host + (options.port - 80 === 0 ? '' : ':' + options.port);

line 589: req.headers.host = remoteHost;
line 590: req.headers.origin = 'http://' + options.host;

line 589: req.headers.host = remoteHost;
new line 590: req.headers.origin = 'http://' + remoteHost ;

i tried this with chrome, it diplays the headers like this bellow
Host: localhost:1800
Origin:http://localhost:1800

i've looked into the code from socket.io, i'm convinced, that this should work after these changes.

regards

[dazagrohovaz]

Hi, again,

sorry about my stupid anwser, i tried it, and don't work. :(

but this problem was open at issue #34

andyichr commented:
#34 (comment)

i didn't kill both lines, just line 590, and then works.

• req.headers.origin = 'http://' + options.host;
• //req.headers.origin = 'http://' + options.host;

regards

[dazagrohovaz]

Feeback:

i tried this with the line 590 commented; after few clicks, i become other error at chrome, i test with both lines commeted, works too, but after few clicks i become an error in node

(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.

13 May 20:08:47 - [ERROR] Trace
Trace:
at Socket. (events.js:123:17)
at onUpgrade (/usr/local/lib/node/.npm/http-proxy/0.5.0/package/lib/node-http-proxy.js:570:18)
at Agent. (/usr/local/lib/node/.npm/http-proxy/0.5.0/package/lib/node-http-proxy.js:610:5)
at Agent.emit (events.js:81:20)
at Socket. (http.js:1240:14)
at Socket._onReadable (net.js:678:27)
at IOWatcher.onReadable as callback

regards

[jfis]

thanks for trying this. it didn't work because you got origin mismatch or data messages were not being duplicated?

in the original post i noted that "(you may need to modify node-http-proxy to not change headers)"
that's if you get the origin mismatch error.
i should have pointed to the issue.
what i've done is commented out the lines where the headers are changed. (589,590)

i believe this issue is unrelated.
do you have all the same versions?
after getting around the mismatch, and opening > 1 browsers to the proxied path(http://localhost:8000), hit send on the first one(the first client to connect) and the browser console and server console will show this message being duplicated (# of clients times). you should also see the connections drop after ~10secs on a heartbeatTimeout in socket.io.

if you do the same thing but go to the non-proxied path( http://localhost:9000), messages aren't duplicated and connections don't seem to close.

//line 605 or so
agent.on('upgrade', function(request, remoteSocket, head) {
// Prepare socket
_socket(remoteSocket, true);

// Emit event
onUpgrade(remoteSocket);
});

agent emits upgrade for each client connection. in onUpgrade, event listeners are added to the socket and remoteSocket. because agent is the same object across clients, this gets called multiple times per client. if there are 2 connections, connection 1 will have 2 listeners for 'data' and will send duplicates to the remoteSocket.

//in onUpgrade line ~550
socket.on('data', listeners._data = function(data) {
  // Pass data from client to server
  try {
    reverseProxy.write(data);
  }
  catch (e) {
    reverseProxy.end();
    socket.end();
  }
 });

[jfis]

well then you are seeing my issue of duplicate listeners :)
i could not get the same error as you though. my maxListeners must be higher.
this suggests we have different versions.
but the same problem is illustrated nonetheless.
thanks.

[dazagrohovaz]

Hi jfis,

i've taked a look again, now i really understood what you mean, ...
the http-proxy send the response to all clients, i think (i didn't tried it), it's because the http.request function preserve opened connections, and interact with the server like a one simple client, like in RFC xxxx (i cann't remenber the number now) about HTTP/1.1 described is. Now when the event onData is called it write this response back to all clients (in theory).

I had similar problems handling with the method CONNECT, with the proxyjs library, but the objective was something else,
would have to be able to add it in the browser configuration as a forward proxy, reverse functions didn't work planed.

sorry, back to the thema... i looked on the code fron http,js (from ry),

the http.request function call the function getAgent into http.js, this function build for each connection an ID (host + port) and make this Agent persistent, in this case all clients call the same host + port connection in node througth the same agent ( returned fron the request function), if one or more clients (browser) made connections to a server througth the proxy all of them need his own connection from the proxy to the server, the standard HTTP-Server from Node.JS didn't do that, no with the request function, if we didn't create a new agent explicit with, like example "var myAgent = new http.ClientRequest(options);", it would never work properly.

Sorry about all that text, i hope you understand where the problem is. if not, just ask.

regards

[indexzero]

@dazagrohovaz Yes, I saw it once I realized that the agent has an eventemitter leak. The problem here is that the proper multiplexing of request-response data doesn't seem possible in the current http.Agent API design because there is no way to map the closure scoped data on the upgrade event to the incoming http request.

I am going to start seriously considering looking at using a raw TCP proxy for this with my own instantiated HTTPParser.

[dazagrohovaz]

the http.Agent API implement a request object from http.ClientRequest, and i think this one it's what you need within http-proxy. You need to implement something like the Agent API, and build for each client-->proxy-connection a proxy-->server-connectoin and preserve it into your app. there is a map for the http.ClientRequest API.

http://nodejs.org/docs/v0.4.7/api/http.html#http.ClientRequest

regards

[indexzero]

http.ClientRequest is deprecated as is http.Client. They will be removed in future versions of node.js so we shouldn't rely on them.

[indexzero]

@dazagrohovaz @jfis Can you take a look at these repros again after my latest commit? I think I've fixed the event emitter leak by managing the containing the request used to the closure scope of the upgrade event. It's a little tricky, but I think it will solve some of these problems.

Will still have to look into the origin mismatch problem

[jfis]

@indexzero looks good and works for me after getting around the origin mismatch.
very nice.

[indexzero]

w000!!!! Ok, I'm going to make the origin mismatch thing an option and write some more robust tests here. @dazagrohovaz is that map application on your personal site open source? Seems like a great websocket demo to help benchmark / test against node-http-proxy.

[dazagrohovaz]

yes, it is. with some mods, https://github.com/dazagrohovaz/maptail, i don't have commited the changes, but i would do it.
in my version works as module, can be integrated to a http or express server, but require express.

[dazagrohovaz]

ready... now , you can clone it.

if you want to show the really filename, the line 165 need to be fixed at the index.js file

[indexzero]

@dazagrohovaz Thanks! I will try this out later tonight to see if I've fully resolved the issue

[indexzero]

This should be resolved as of v0.5.3. Let me know if you run into any issues. @dazagrohovaz I couldn't get your maptail working ... if you want to try running your site with node-http-proxy instead of squid that would be appreciated.