HTTP Server Response Socket Timeout After Two Minutes Does Not Allow Catching `timeout` event and respond to `socket`

[dscape]

Currently node sets a two minute timeout on a socket by default

socket.setTimeout(2 * 60 * 1000); // 2 minute timeout
socket.once('timeout', function() {
  socket.destroy();
});

http.js

Problem is that once the timeout event is fired the socket is destroyed, making it impossible for modules using http to intercept the event:

var http = require('http');

http.createServer(function (req, res) {
  res.socket.setTimeout(10);
  res.socket.once('timeout', function () {
    res.writeHead(200, {'Content-Type': 'text/plain'});
    res.end('Hello World\n');
  });
}).listen(1337, '127.0.0.1');

However when you curl you get:

$ curl localhost:1337
curl: (52) Empty reply from server

Which is not the expected behavior.

A work-around is to do:

var http = require('http');

http.createServer(function (req, res) {
  res.socket.setTimeout(10);
  res.socket.removeAllListeners('timeout');
  res.socket.once('timeout', function () {
    res.writeHead(200, {'Content-Type': 'text/plain'});
    res.end('Hello World\n');
  });
}).listen(1337, '127.0.0.1');

Which works correctly, but at impossible to predict consequences as this is userland code and node-core might break this in the future:

$ curl localhost:1337
Hello World

A solution was indicated to me by @isaacs stating changing to process.nextTick semantics could easily solve this:

socket.setTimeout(2 * 60 * 1000); // 2 minute timeout
socket.once('timeout', function() {
  process.nextTick(socket.destroy);
});

http-modified.js

+@mmalecki

[bnoordhuis]

I agree the current API is not well thought out.

@isaacs Are you still planning to do a HTTP rewrite in v0.9?

[isaacs]

Yes, that is my goal. Let's collect up these issues so that we have a good set of requirements.

[dscape]

In case you didn't notice the test (and tentative fix) are at 7a79de3

[isaacs]

Let's revisit for 0.12.

This bug is breaking code in production. Can we get a fix in sooner?

[isaacs]

I'm ok with fixing in 0.10 if it can be done relatively simply and without
changing the default behavior. What API do you have in mind? Want to send a
patch?

On Saturday, February 9, 2013, James Halliday wrote:

This bug is breaking code in production. Can we get a fix in sooner?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/3460#issuecomment-13331440..

[bnoordhuis]

Suggestion: req.setTimeout(timeout, [callback])? That's what we use everywhere else.

By the way, the 2 minute timeout is to stop a client that doesn't send headers from tying up a connection indefinitely. You don't get a request object until the headers have been received so that initial timeout can't go - at best, it can be made configurable. req.setTimeout() will need to disarm it, though.

[isaacs]

Oh, right, that makes sense. A 2-minute default is probably a good idea there. But, it could be based on a field on the server. Maybe server.setTimeout() in order to set the (pre-'request' event) connection timeout? It's kinda weird, but at least wouldn't be a magic number buried in the source code that could never be changed.

We could also probably move things around so that blerg.setTimeout(n, cb) will always reliably change what the timeout actually is. req.setTimeout(1000*60*60*24*365) should make requests not ever get killed unless they're a year old. req.setTimeout(Infinity) would let them literally stay alive forever with no traffic.

[bnoordhuis]

req.setTimeout(Infinity) would let them literally stay alive forever with no traffic.

net.Socket#setTimeout does that when you pass in msecs=0. I would suggest to stick with that.