Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Socket setTimeout behaviour not working as expected for HTTP requests #5899

Closed
mtharrison opened this issue Mar 25, 2016 · 17 comments
Closed
Labels
http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem.

Comments

@mtharrison
Copy link
Contributor

Summary

timeout events are fired on sockets assigned to an HTTP request even when there is activity (data being written) within the timeout period. This is unexpected and not explicitly documented.

This did not used to be the case, it used to work like a normal net socket, with the timeout being essentially "reset" on any activity. The behaviour changed after this commit 607bbd3 without any obvious doc changes.

Full background

A TCP (net) socket has a setTimeout method which sets the socket to emit a timeout event after a certain number of ms of inactivity. This currently works correctly on all tested versions. An example is below:

var net = require('net');

var server = net.createServer(function (socket) {
    socket.setTimeout(200, socket.destroy);              // Setting to less than 100 will cause timeout
    socket.pipe(process.stdout);
});

server.listen(function () {
    var client = net.connect(server.address().port, function () {
        var interval = setInterval(function () {
            client.write('a');
        }, 100);
        setTimeout(function () {
            clearInterval(interval);
            client.end();
            server.close();
        }, 1000);
    });
});

There’s also a setTimeout method available on a HTTP request which proxies the setTimeout call through to the underlying TCP socket. So I expected it to work the same. But it does not, it will timeout even when data is being written actively to the socket.

var http = require('http');

var server = http.createServer(function (req, res) {

    req.setTimeout(200);                              // this will timeout even though data is being written
    req.pipe(process.stdout);
    req.on('end', function () { res.end() });
});

server.listen(function () {

    var address = server.address();
    var req = http.request({
        port: address.port,
        method: 'POST'
    });

    var interval = setInterval(function () {
        req.write('a');
    }, 100);
    setTimeout(function () {
        clearInterval(interval);
        req.end();
        server.close();
    }, 1000);
});

It was an issue posted at hapijs/hapi#3099 that led me to this digging. I used the script above to git bisect the Node core repo back to the commit where the behaviour changed 607bbd3 which is part of PR #2355.

Conclusion

I think this is either a bug or If the new behaviour is intended, a documentation update is probably needed to explicitly state that the timeout on an http socket works differently than a regular net socket.

  • Version: 607bbd3 - master
  • Platform: Darwin 15.3.0 Darwin Kernel Version 15.3.0: Thu Dec 10 18:40:58 PST 2015; root:xnu-3248.30.4~1/RELEASE_X86_64 x86_64
  • Subsystem: net, http
@mscdex mscdex added http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. labels Mar 25, 2016
@silverwind
Copy link
Contributor

Dupe of #3319 maybe?

edit: closed the other issue as this one has more detail.

@silverwind
Copy link
Contributor

@indutny

@indutny
Copy link
Member

indutny commented Mar 30, 2016

cc @nodejs/http

@alanstott
Copy link

alanstott commented Apr 17, 2016

+1

@indutny
Copy link
Member

indutny commented Apr 19, 2016

Confirmed, caused by stream consumption indeed. Working on a fix, thanks!

indutny added a commit to indutny/io.js that referenced this issue Apr 19, 2016
When underlying `net.Socket` instance is consumed in http server - no
`data` events are emitted, and thus `socket.setTimeout` fires the
callback even if the data is constantly flowing into the socket.

Fix this by calling `socket._unrefTimer()` on every `onParserExecute`
call.

Fix: nodejs#5899
@indutny
Copy link
Member

indutny commented Apr 19, 2016

Fix #6286

MylesBorins pushed a commit that referenced this issue Apr 20, 2016
When underlying `net.Socket` instance is consumed in http server - no
`data` events are emitted, and thus `socket.setTimeout` fires the
callback even if the data is constantly flowing into the socket.

Fix this by calling `socket._unrefTimer()` on every `onParserExecute`
call.

Fix: #5899
PR-URL: #6286
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 21, 2016
When underlying `net.Socket` instance is consumed in http server - no
`data` events are emitted, and thus `socket.setTimeout` fires the
callback even if the data is constantly flowing into the socket.

Fix this by calling `socket._unrefTimer()` on every `onParserExecute`
call.

Fix: #5899
PR-URL: #6286
Reviewed-By: James M Snell <[email protected]>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
When underlying `net.Socket` instance is consumed in http server - no
`data` events are emitted, and thus `socket.setTimeout` fires the
callback even if the data is constantly flowing into the socket.

Fix this by calling `socket._unrefTimer()` on every `onParserExecute`
call.

Fix: nodejs#5899
PR-URL: nodejs#6286
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue Apr 26, 2016
When underlying `net.Socket` instance is consumed in http server - no
`data` events are emitted, and thus `socket.setTimeout` fires the
callback even if the data is constantly flowing into the socket.

Fix this by calling `socket._unrefTimer()` on every `onParserExecute`
call.

Fix: #5899
PR-URL: #6286
Reviewed-By: James M Snell <[email protected]>
@baolinCloud
Copy link

I met the same issue with node-v0.10.36, should I upgrade the node to solve this problem? Any other ideas?

@cjihrig
Copy link
Contributor

cjihrig commented May 16, 2016

Yes. This likely won't make it back to the 0.10 release line.

@thomas-riccardi
Copy link

@baolinCloud node-v0.10 doesn't include the initial commit 607bbd3 that introduced the regression. lib/http.js hasn't changed on things related to timeout since before this regression commit: https://github.com/nodejs/node/commits/v0.10/lib/http.js
And we only encountered the issue when starting using node v4.

So I think this bug is not present in node v0.10.

@thomas-riccardi
Copy link

nodejs v4.4.4 was released after this fix landed on master, but didn't include this fix.
It was released in v5.11.0 though (although with no explicit comment explaining that setTimeout was broken...), and v6.0.0.

When will this be released in v4.x ?

This is a severe regression for all users of setTimeout.

@jasnell
Copy link
Member

jasnell commented May 17, 2016

v4.4.4 was intentionally limited to a smaller set of security related fixes. I imagine this can hit in v4.4.5. /cc @thealphanerd

@MylesBorins
Copy link
Contributor

I just push this change to v4.x-staging and it will be included in v4.4.5

Expect an rc later today and a release next week

MylesBorins pushed a commit that referenced this issue May 17, 2016
When underlying `net.Socket` instance is consumed in http server - no
`data` events are emitted, and thus `socket.setTimeout` fires the
callback even if the data is constantly flowing into the socket.

Fix this by calling `socket._unrefTimer()` on every `onParserExecute`
call.

Fix: #5899
PR-URL: #6286
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue May 18, 2016
When underlying `net.Socket` instance is consumed in http server - no
`data` events are emitted, and thus `socket.setTimeout` fires the
callback even if the data is constantly flowing into the socket.

Fix this by calling `socket._unrefTimer()` on every `onParserExecute`
call.

Fix: #5899
PR-URL: #6286
Reviewed-By: James M Snell <[email protected]>
@baolinCloud
Copy link

@triccardi-systran My test code is simple:

var http = require('http');

server = http.createServer(function (request, response) {
setTimeout(function() {
response.writeHead(200, {'Content-Type': 'text/plain'});
response.end('Hello World\n');
}, 200);
request.socket.setTimeout(500);
request.socket.removeAllListeners('timeout');
request.socket.once('timeout', function () {
response.writeHead(200, {'content-type': 'text/html'});
response.end('hello world');
console.log('timeout');
});
}).listen(1337, '127.0.0.1');

When I use postman to get from http://127.0.0.1:1337 , I get the response of 'Hello World'. But also log 'timeout'. My node version is 0.10.36. Any thing is wrong with my code? I just want to set timeout to less than 2min.

@mtharrison
Copy link
Contributor Author

mtharrison commented May 18, 2016

@baolinCloud I think that's expected behaviour. You responded and ended the first request in the 200ms timeout, then the socket (still connected because of Keep-Alive) times out.

If you just want to change the timeout on the server to less than 2 min use server.setTimeout():

var http = require('http');

server = http.createServer(function (request, response) {

    // set to more than 200ms and no response will come back because of timeout

    setTimeout(function() {
        response.writeHead(200, {'Content-Type': 'text/plain'});
        response.end('Hello World\n');
    }, 200);

}).listen(1337, '127.0.0.1');

server.setTimeout(500);

@baolinCloud
Copy link

@mtharrison I want to catch the timeout event and response a specified message.

For example, if time out , log the event and response 'hello'. Otherwise, response 'HELLO'.

And I don not want to log timeout if not timeout.

In fact I always get the timeout log whether timeout or not.

Any ideas ?

@matthiasg
Copy link

This seems to happen again in node 14. Really bad behavior. Uploads stop at about 1m with ECONNRESET

@kanongil
Copy link
Contributor

@matthiasg You should open a new issue with details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.