crash in accessing res.connection.pair #62

Closed
qzaidi opened this Issue Jun 14, 2011 · 29 comments

Comments

Projects
None yet
7 participants

qzaidi commented Jun 14, 2011

Hi,

I am using the latest http-proxy in hostNameOnly mode and it kept crashing in
https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L372

I dont' have the stack trace right now, but it said something like res.connection was undefined.
. I just changed res to req in that line and everything started to work. I am very new to nodejs, so not sure what may be happening, but is this just a typo? I would think you would use the request object to set the header on the proxied request, so it does seem like one.

Regards
Qasim

Owner

indexzero commented Jun 14, 2011

@qzaidi The hostNameOnly was a contributed feature, so additional information about this issue (such as a stack trace or some sample code that reproduces the issue) would be quite helpful.

What version of node, npm, and node-http-proxy are you currently using?

Contributor

Marak commented Jun 14, 2011

Also, a confirmation of running the unit tests would be good. Thanks!

qzaidi commented Jun 14, 2011

Here's a stack trace

/usr/local/lib/node_modules/http-proxy/lib/node-http-proxy.js:357
req.headers['x-forwarded-proto'] = res.connection.pair ? 'https' : 'http';
^
TypeError: Cannot read property 'pair' of undefined
at [object Object].proxyRequest (/usr/local/lib/node_modules/http-proxy/lib/node-http-proxy.js:357:52)
at Server. (/usr/local/lib/node_modules/http-proxy/lib/node-http-proxy.js:155:13)
at Server.emit (events.js:67:17)
at HTTPParser.onIncoming (http.js:1123:12)
at HTTPParser.onHeadersComplete (http.js:108:31)
at Socket.ondata (http.js:1018:22)
at Socket._onReadable (net.js:683:27)
at IOWatcher.onReadable as callback

I am using node 0.4.8, npm 1.0.10

Owner

indexzero commented Jun 15, 2011

Can you provide some sample code? I am unable to reproduce this.

I can confirm this error occurs however it is very random in nature. My program can be running for 10 seconds or 10 minutes without hitting this problem and I can't seem to find any reliable way of replicating it during testing. It's only when under live load it falls.

I hard coded that line so it always used http as we don't accept https traffic just to see what happened and of course it no longer erred there but it did further down at

response.on('data', function (chunk) {
  if (req.method !== 'HEAD') {
    res.write(chunk);
  }
});

I got a socket can't be written to error. So I'm guessing in my case at least in some rare instances node is losing the connection to the user. Is this normal?

Owner

indexzero commented Jun 26, 2011

@daemon-byte, are you using HTTPS? I do think that there are some cases in which HTTPS can cause a socket wake up and become unwriteable.

No, as I said we only accept http on our nodejs server. It's just there to sit in front of varnish on our public facing site.

Owner

indexzero commented Jun 26, 2011

@daemon-byte So I'm going to close this as wont fix. I made a fix (which you can see here: https://gist.github.com/1047764), but after running the benchmarks with this applied the additional overhead of the try/catch logic seems to be degrading performance by 10-15%.

My suggestion would be to use a standalone instance of the HttpProxy object and wrap the entire call to .proxyRequest() in a try/catch.

indexzero closed this Jun 26, 2011

bpierre commented Jul 16, 2011

Same here.

I do not understand what exactly triggers the crash, and the wont fix. Could you give more details?

There is a performance issue, sure, but a crash does not look like a viable alternative to me :-)

To be honest bpierre I haven't figured out the exact trigger and it's proving to be a nightmare. Best I can do currently is throw a 500 error out to the client but I can't put a system live like that. When I divert live traffic through it then there are just to many instances of it happening for us to accept. It's only a few percent but frequent enough to be a problem.

Irrelon commented Jul 25, 2011

I have found a trigger for it. Whenever a client using Firefox 4 or 5 connects the error occurs. It also occurs sometimes when connecting with Opera. It is triggered without fail when FF 4 or 5 connects though.

At the other end of my proxy is a node.js instance that serves files to the client. On each occasion it will serve: index.htm, then main.css and as soon as it tries to serve the first .png file, the proxy crashes.

Is anyone else serving a .png file when this occurs?

Mine tends to be the css sheets but it has happened with images. So far I haven't seen it trigger on FF but there was 2 versions of opera (although I used one of those versions and it didn't crash) and a couple of mobile phone browsers. All I can think off is that at some point the browser is making subsequent connections for extra files and losing the connection somewhere. Maybe it has to do with modern browsers making multiple connections at the same time.

Contributor

Marak commented Jul 25, 2011

@daemon-byte @bpierre - To my understanding this issue only affects hostNameOnly mode. If you want a more robust proxy, you should be using the lower level APIs we expose ( like @indexzero has suggested ).

It's been a while since I looked at the code so I am not exactly sure what you mean by hostnameonly mode and thus if that is the only thing. However it does not bother me where the try catch statement is put, what bothers me is there is no way to recover it once you reach that point. The best you can do is throw an error code out to the user which is far from ideal. I can't have 5% of our traffic generating 500 errors thus I need to understand what is happening and address the problem rather than just put a bodge around it.

Contributor

Marak commented Jul 25, 2011

Well, unless you can reproduce the error, there isn't much that can be done, sorry.

I wasn't expecting that. I was just discussing the problem with similar sufferers with the hope we could track down the issue in details.

Owner

indexzero commented Jul 25, 2011

@daemon-byte @bpierre @coolbloke1324 This issue is marked wont-fix because there is nothing we can do to fix it in the node-http-proxy project itself; it is blocked by a core node.js bug which is not scheduled for fix until 0.6.x.

In the meantime, a recent feature has been added which may allow you work around this issue if you don't use the x-forwarded-* headers. You can now pass in enableXForwarded: false in the options to HttpProxy instances and .createServer() and this will bypass the check which is throwing the exception: https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L385

You will of course not get the x-forwarded-* headers in your target application, but if that's not a concern for you this do the trick.

Owner

indexzero commented Jul 25, 2011

@dominictarr can shed some light on whether this is pushed in the latest version of node-http-proxy on npm

Owner

indexzero commented Jul 25, 2011

@dominictarr FYI:

$ npm search http-proxy
http-proxy  A full-featured http reverse proxy for node.js   =marak =indexzero =dominictarr reverse proxy http

Irrelon commented Jul 25, 2011

@indexzero thanks for the details. Don't suppose you know the node.js bug tracker url do you? Interested to know what is going on under the hood with this one!

Owner

indexzero commented Jul 25, 2011

@coolbloke1324 Very busy preparing for upcoming releases, don't have the time to dig around to find it. Try searching for HTTPS releated bugs in the nodejs issues: http://github.com/joyent/node/issues

Contributor

dominictarr commented Jul 26, 2011

@daemon-byte @bpierre @coolbloke1324 @indexzero

the feature indexzero refers to in

In the meantime, a recent feature has been added which may allow you work around this issue if you don't use the x-forwarded-* headers. You can now pass in enableXForwarded: false in the options to HttpProxy instances and .createServer() and this will bypass the check which is throwing the exception: https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L385

You will of course not get the x-forwarded-* headers in your target application, but if that's not a concern for you this do the trick.

is now available on npm in http-proxy@0.6.0

I guessed it might have an underlying bug which was causing an issue. It's a shame that the bug is not scheduled for the next big release. Is there any sort of time line for that? Also judging from your comment does that mean the issue only triggers off when someone tries to use ssl traffic because we don't use https.

bpierre commented Jul 26, 2011

I tried with http-proxy@0.6.0 and enableXForwarded: false, it crashed too:

net.js:334
      throw new Error('Socket.end() called already; cannot write.');
            ^
Error: Socket.end() called already; cannot write.
    at Socket.write (net.js:334:13)
    at ServerResponse._writeRaw (http.js:392:28)
    at ServerResponse._send (http.js:372:15)
    at ServerResponse.end (http.js:687:18)
    at IncomingMessage.<anonymous> (/srv/http/node-proxy/node_modules/http-proxy/lib/node-http-proxy.js:484:13)
    at IncomingMessage.emit (events.js:81:20)
    at HTTPParser.onMessageComplete (http.js:133:23)
    at Socket.ondata (http.js:1227:22)
    at Socket._onReadable (net.js:683:27)
    at IOWatcher.onReadable [as callback] (net.js:177:10)

And here is the script:

#!/usr/bin/node

var http = require('http'),
    httpProxy = require('http-proxy'),
    fs = require('fs');

var proxy = new httpProxy.HttpProxy();

var server = httpProxy.createServer(function (req, res) {
  if (req.headers.host === 'domain-with-websocket.com') {
    proxy.proxyRequest(req, res, {
      host: '127.0.0.1',
      port: 3000,
      enableXForwarded: false
    });
  } else {
    // Other domains (Nginx backend)
    proxy.proxyRequest(req, res, {
      host: 'localhost',
      port: 8080,
      enableXForwarded: false
    });
  }
});

server.on('upgrade', function(req, socket, head) {
  if (req.headers.host === 'domain-with-websocket.com') {
    proxy.proxyWebSocketRequest(req, socket, head, {
      host: 'localhost',
      port: 3000
    });
  }
});

server.listen(80);
Contributor

dominictarr commented Jul 26, 2011

that looks normal. what about the backend? can you reduce that to a minimal failing example?

that error message sounds like maybe one of the sockets (client-proxy or proxy-server) is closing at a diffirent time to the other.

bpierre commented Jul 26, 2011

I reduced it to:

var httpProxy = require('http-proxy');

var proxy = new httpProxy.HttpProxy();

var server = httpProxy.createServer(function (req, res) {
  proxy.proxyRequest(req, res, {
    host: 'localhost',
    port: 8080,
    enableXForwarded: false
  });
});

server.listen(80);

I’m waiting for the next crash now, I keep you informed.

Contributor

dominictarr commented Jul 26, 2011

sorry, by backend I meant what is the server like which is being proxied?

bpierre commented Jul 26, 2011

It’s a Nginx server, with various fcgi (php-fpm) and uwsgi (Django) apps.

But since the above update (I removed the Node / Socket.IO server), the proxy has not crashed… It seems to be related to my Node / Socket.IO app.

I will try to launch another node-http-proxy for the Node app to confirm that.

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