Uncatchable error when upstream server returns malformed HTTP response #347

Closed
jcheng5 opened this Issue Dec 7, 2012 · 6 comments

Projects

None yet

3 participants

@jcheng5
jcheng5 commented Dec 7, 2012

We have a production app that uses node-http-proxy running in front of some (admittedly sketchy) webservers running R code. (http://www.r-project.org) We've been seeing our node process crash several times a day. It turns out to be a bug in node with a known workaround but node-http-proxy prevents us from using that workaround.

There was a bug in the R code that caused the Content-Length to be inaccurately reported upstream. The issue here though is that the content-length caused Node issue 3776 to come into effect; when an http request is made and the node http parser encounters an error, that error is emitted on the socket/connection object. In the case of node-http-proxy's proxyRequest method, the caller can never catch that error, and therefore the node process exits. Obviously we would rather gracefully deal with the error than have the process exit.

Issue 3776 was fixed in Node 0.9.2 but since Node 0.8.15 is still the latest stable version, it would be great if this bug could be worked around in node-http-proxy as well. I'm about to submit a pull request that approximates the Node 0.9.2 fix.

@jcheng5 jcheng5 added a commit to rstudio/node-http-proxy that referenced this issue Dec 8, 2012
@jcheng5 jcheng5 Forward errors that occur on response.connection
Without this commit, Node versions 0.9.1 and earlier cause the process to
terminate with no recourse if upstream HTTP servers return malformed HTTP
responses.

Fixes issue #347
4252933
@indexzero
Member

Seems reasonable. Wondering what @isaacs thinks about this.

@mmalecki

@jcheng5 we've seen this in production a few days ago. I'm not really comfortable with admitting that the fix was:

var destroy = net.Socket.prototype.destroy;
net.Socket.prototype.destroy = function (err) {
  console.dir(err); // and some moar logging
  destroy.call(this, null);
};

I think that on 0.9.x this is related to process.nextTick refactor which makes next tick happen before I/O, but no idea on the cause in 0.8.x.

@isaacs opinions?

@mmalecki

This appears to be fixed by nodejs/node-v0.x-archive@827b2a9.

@jcheng5
jcheng5 commented Dec 21, 2012

@mmalecki Great news, thanks! Looking forward to v0.8.17!

@jcheng5 jcheng5 closed this Dec 21, 2012
@mmalecki

@jcheng5 I just pushed a custom build branch, in case you wanted to use it.

@jcheng5
jcheng5 commented Dec 22, 2012

Great, thank you @mmalecki, much appreciated!

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