No way to close proxy/server websocket on client/server error

[glasser]

The ws pass does not have any logic to say "if an error occurs on socket, clean up proxySocket somehow". And there's no way for you to add that at the application level, because proxySocket is never passed to application code. This means that in practice, our proxies leak proxySockets.

Yes, if socket is cleanly closed, the pipe will close proxySocket as well. But that doesn't occur if socket errors; for example, if the client disappears from the network (without closing its TCP sockets), socket will emit error, and proxySocket will never be cleaned up.

See https://github.com/glasser/proxy-error-handling for a reproduction of this issue; it is trivially reproduced if you have a second machine (such as a recent Android/iPhone) that can load a web page served from your workstation and then disconnect itself from the network.

I've found a hacky way to close proxySocket: calling socket.unshift(null) causes socket to emit end, which triggers proxySocket.end() through the pipe. But this is an undocumented use of unshift. Surprisingly, socket.destroy() does not seem to cause socket to emit end and has no effect on proxySocket.

Node 0.10.22, but I don't believe there are any net or streams changes in 0.10.23 or 0.10.24.

See also meteor/meteor#1769.

[jcrugzz]

@glasser thanks for your thorough report. This definitely seems fixable and I will try and give it a look later today. If you are able to dig deeper into a possible solution please post here :)

cc @yawnt

[glasser]

@jcrugzz @yawnt

Here's my thought. There are two ways we could go. Either http-proxy should itself take care of this, by reacting to 'error' on socket by ending proxySocket... or http-proxy should give the end user access to proxySocket and let it do its own error handling.

I think the former is probably the way to go. First of all, it's more or less necessary to do this error handling, and we shouldn't make it easy for users to accidentally cause a leak. Secondly, it's already the case that the ws stream phase doesn't give much control over the way that socket and proxySocket are connected (eg, it doesn't let you interpose some sort of transformation). If we add an API for that later (or the user overrides the stream phase entirely) then sure, it would make sense to also give the user control over the error handling semantics. But because http-proxy doesn't offer that control, then it makes sense to do the automatic right thing.

Working on a PR now. One thing that confuses me: it looks like there's a straight-up bug in the stream pass: it takes its server and head arguments in a different order from how they are passed in! Is there a test suite that should be catching this? (Our current production use of http-proxy/caronte is based on an old commit from back when caronte used EventEmitter2 which doesn't have this issue.)

[glasser]

OK, see #560 for the "one thing that confuses me". I'm going to build a PR on top of that PR to fix this issue.

[glasser]

#561 is the PR for this issue.

Note that even with this PR, it's most likely the case that most users will also want to run

proxy.on('error', function (err, req, socket) {
  socket.end();
});

which we've had in our code forever. That cleans up the other direction: errors in outgoing should close incoming. You might want to add that to the ws stream pass too. The main difference is that users actually can write the above code with the current module, whereas they can't write the code that's being added in this commit. (Also, you could imagine wanting to have different error handling behavior for this case: for example, maybe the proxy should instead to try reconnect to another backend server while keeping the client's socket alive.)