http.request timeout doesn't take effect

[aleung]

• Version: 7.4.0
• Platform: MacOS 10.11.6
• Subsystem: http

Since node.js 6.8.0, http.request supports timeout option. (api-doc)

I test this feature with below code:

'use strict';

var http    = require('http');

const server = http.createServer((req, res) => {
    console.log(Date.now(), 'Server get incoming request');
    setTimeout(() => {  // delay 10 seconds
        console.log(Date.now(), 'Server send response');
        res.writeHead(200, {'Content-Type': 'text/plain'});
        res.write('Hello');
        res.end();
    }, 10000);
});
server.listen(8000);

const req = http.get({
    hostname: '127.0.0.1',
    port: 8000,
    path: '/',
    timeout: 100  // timeout in 0.1 second
}, function (response) {
    // not expected
    console.log(Date.now(), 'Get response from server');
    process.exit();
});

req.on('error', (err) => {
    console.log(Date.now(), 'Error', err);
    process.exit();
});

Output I got:

1490270444601 'Server get incoming request'
1490270454604 'Server send response'
1490270454608 'Get response from server'

The response was delayed 10 second to send and I expected a ETIMEOUT on http client. But it didn't.

What's more, I read the file change in #8101 which is PR adding timeout to http.request, the test case seems not testing http.request's timeout option.

[neroux]

timeout appears to refer to the connect timeout, not read timeout

timeout : A number specifying the socket timeout in milliseconds. This will set the timeout before the socket is connected.

Setting a timeout on the underlying socket might possibly work for you.

req.on('socket', (s) => { s.setTimeout(100, () => { s.destroy(); })});

[aleung]

@neroux As document says: it specifies the socket timeout. Socket timeouts after a duration of inactivity on the socket. It should cover both connect timeout and read timeout.

[neroux]

The term socket timeout does not specify as to what times out. In this case it refers to the connection.

[bnoordhuis]

What @neroux said. To set the read timeout, use req.setTimeout(). Closing, working as documented and intended.

[aleung]

It makes me confusing.

@lpinca gives me comments in nock/nock#754 (comment) that it checks if there is socket activity for the whole duration of the request.

If it's as what @neroux said: it refers to the connection only, how to explain this code section which was committed in PR #8101 by @reneweb and PR #9440 by @italoacasas?

  if (req.timeout) {     // req.timeout = options.timeout
    const emitRequestTimeout = () => req.emit('timeout');
    socket.once('timeout', emitRequestTimeout);
    req.once('response', (res) => {
      res.once('end', () => {
        socket.removeListener('timeout', emitRequestTimeout);
      });
    });
  }
  req.emit('socket', socket);

[aleung]

To make sure, I have read through the source code net.js, _http_client.js and _http_agent.js.

First, when you call socket.setTimeout() before a socket is connect, the idle timer is kept until the socket is destroyed or setTimeout(0) is called. A socket won't clear its idle timer when it gets connect.

The timeout parameter in option is passing through from http.request to http.Agent, then to net.createConnection and finally set on Socket. And I trace the connect event route and can't find any place that removes the timer.

[bnoordhuis]

Okay, I'll reopen. Asked the reviewers to chime in: #8101 (comment)

I'll note that the pull request has some red flags (trailing whitespace, long lines) that suggest it could have been scrutinized more.

[mcollina]

I think this is a documentation problem more than a code problem. The PR does what is supposed to be doing. If we check https://nodejs.org/api/net.html#net_socket_settimeout_timeout_callback, the connection is not severed, but a 'timeout'  event is emitted.

This is what https://github.com/nodejs/node/blob/master/test/parallel/test-http-client-timeout-option.js#L25-L29 proves.
#9440 does its job at preventing the memory leak.

[aleung]

@mcollina I'm not a native English speaker. Maybe I don't really understand this sentence:

When an idle timeout is triggered the socket will receive a 'timeout' event but the connection will not be severed.

It doesn't apear to me that after connection is established the idle timeout will be disabled.

Let's confirm with code:

const net = require('net');

const PORT = 5001;

const server = net.createServer( (socket) => {
    console.log(Date.now(), 'SERVER connected');
    setTimeout(() => {      // delay 10 seconds
        console.log(Date.now(), 'SERVER send data');
        socket.write('hello\r\n');
    }, 10000);
});

server.on('error', (err) => {
    console.log(Date.now(), 'SERVER error', err);
});

server.listen(PORT);

console.log(Date.now(), 'CLIENT create connection');
const client = net.createConnection({
    port: PORT,
    timeout: 100
}, () => {
    console.log(Date.now(), 'CLIENT connected');
});

client.on('timeout', () => {
    console.log(Date.now(), 'CLIENT timeout');
    process.exit();
});

client.on('data', (data) => {
    console.log(Date.now(), 'CLIENT received data', data.toString());
    process.exit();
})

After connection established, socket server delays 10 second to send data. Client sets timeout to 100ms and it gets timeout event.

1490416283744 'CLIENT create connection'
1490416283756 'CLIENT connected'
1490416283758 'SERVER connected'
1490416283856 'CLIENT timeout'

From this test result on net.Socket, it proves that socket timeout checks if there is socket activity for the whole duration of the connection.

[lpinca]

@aleung yes, that confirms what @mcollina wrote.
If the socket stays in idle for more than timeout time a 'timeout' event will be emitted on the socket without closing (severing) the connection.

[aleung]

@lpinca Thanks for the explaination. However, I don't think I get an answer about whether my issue is valid or not.

My test case shows that 'timeout' event is not got on ClientRequest when I use http.request with timeout option. If I'm not using http but am using plain socket, 'timeout' event is triggered on read timeout. The behaviour of using timeout option in these two scenrioes are different.

Is it an issue on http module? Or it's intended? If it's intended, should the document of http.request be updated to state that the timeout is only for connection estiblishing but not for read?

Anyway, I believe when someone uses http.request and specfies timeout option, what he expects is to capture any timeout on the request till it ends rather then only connection estiblishing.

[lpinca]

@aleung I think it works as expected. The 'timeout' event is correctly emitted. It's up to the developer to abort the request.

'use strict';

const http = require('http');

const server = http.createServer((req, res) => {
  console.log(Date.now(), 'Server get incoming request');

  setTimeout(() => {
    console.log(Date.now(), 'Server send response');
    res.writeHead(200, {'Content-Type': 'text/plain'});
    res.write('Hello');
    res.end();
  }, 1000);
});

server.listen(8080, () => {
  const req = http.get({ port: 8080, timeout: 100 });

  req.on('response', () => { throw new Error('Test invalidation'); });
  req.on('timeout', () => req.abort());
  req.on('error', (err) => {
    if (req.aborted) return;

    console.error(err.stack);
  });
});

Documentation should be updated to reflect the actual behavior though.

[aleung]

Sorry everybody, I make a really stupid error in my test case: I want to listen on the timeout event but I wrote error.

@lpinca thank you very much for your kindness answer.

[manoharreddyporeddy]

Here is an working example:

https://stackoverflow.com/a/47546591/984471

[loretoparisi]

@manoharreddyporeddy in that example I see req.destroy(); on timeout and then a check on req.connection.destroyed. Is the same to do req.abort() and therefore checked as aforementioned req.aborted?

[manoharreddyporeddy]

I don't exactly recall what I wrote at that time.

However I see that I mentioned the links in the code in link mentioned, which points to nodejs github source and its tests.

I recall that I tried to compare just like you did, (since we both did same, it's possible that it is correct, but I din't test it), but nodejs docs din't exactly mentioned that, at that time.

So, all I can say is,
we are talking about some documented and un-documented stuff
I have code mentioned in the link, running in production now, so possibly safe
(at least till something breaks in future release, if that is so the link I mentioned will not be saying so.)