Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

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

Closed
dscape opened this issue Jun 17, 2012 · 12 comments
Labels
Milestone

Comments

@dscape
Copy link

dscape commented Jun 17, 2012

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

dscape added a commit to dscape/node that referenced this issue Jun 17, 2012
@bnoordhuis
Copy link
Member

I agree the current API is not well thought out.

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

@isaacs
Copy link

isaacs commented Jul 4, 2012

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

@isaacs isaacs closed this as completed Jul 4, 2012
@isaacs isaacs reopened this Jul 4, 2012
@dscape
Copy link
Author

dscape commented Jul 4, 2012

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

@isaacs
Copy link

isaacs commented Dec 30, 2012

Let's revisit for 0.12.

@ghost
Copy link

ghost commented Feb 9, 2013

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

@isaacs
Copy link

isaacs commented Feb 9, 2013

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
Copy link
Member

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
Copy link

isaacs commented Feb 10, 2013

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
Copy link
Member

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.

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Mar 4, 2013
This adds the following to HTTP:

* server.setTimeout(msecs, callback)
  Sets all new connections to time out after the specified time, at
  which point it emits 'timeout' on the server, passing the socket as an
  argument.
  In this way, timeouts can be handled in one place consistently.
* req.setTimeout(), res.setTimeout()
  Just aliases to req.socket.setTimeout() or res.socket.setTimeout(),
  but without having to delve into a "buried" object.
* server.timeout
  Number of milliseconds before incoming connections time out.
  (Default=1000*60*2, as before.)

Furthermore, if the user sets up their own timeout listener on either
the server or the socket, then the default behavior (destroying the
socket) is suppressed.

Fix nodejs#3460
@isaacs
Copy link

isaacs commented Mar 4, 2013

@substack @dscape @davepacheco Check out #4912. Does that satisfy your needs?

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Mar 6, 2013
This adds the following to HTTP:

* server.setTimeout(msecs, callback)
  Sets all new connections to time out after the specified time, at
  which point it emits 'timeout' on the server, passing the socket as an
  argument.
  In this way, timeouts can be handled in one place consistently.
* req.setTimeout(), res.setTimeout()
  Essentially an alias to req/res.socket.setTimeout(), but without
  having to delve into a "buried" object.  Adds a listener on the
  req/res object, but not on the socket.
* server.timeout
  Number of milliseconds before incoming connections time out.
  (Default=1000*60*2, as before.)

Furthermore, if the user sets up their own timeout listener on either
the server, the request, or the response, then the default behavior
(destroying the socket) is suppressed.

Fix nodejs#3460
@isaacs isaacs closed this as completed in d258fb0 Mar 6, 2013
@tshemsedinov
Copy link

Hello, please read commit comments above: tshemsedinov/node@ae9a1a5
I believe this small fix will not break anything because it just prevent unnatural situation and if someone already have workaround for this case it will be prevented in lower level. Other developers will obtain more predictable behavior.

@sayyedhanif
Copy link

I need to increase the timeout of nodejs request according to particular request dynamically. because request can take any time according to process. suppose one iterate take around 5 min so any particular request can have n number of iterate according to client request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants