Clear timeout when socket is connected

[lpinca]

Right now the "connection" timer is cleared when the response event is emitted.
There are cases where the response event is not emitted, for example if no data is received from the socket after connection.
When this happens timed-out emits a connection timeout error but technically this is not correct as the connection has been established. Here is a test case:

'use strict';

const timeout = require('timed-out');
const http = require('http');

const server = http.createServer();

server.on('request', () => {});

server.listen(() => {
  const req = http.request({
    port: server.address().port,
    hostname: 'localhost'
  });

  timeout(req, 500);

  req.on('error', (err) => console.log(err.stack));
  req.end();
});

It prints the following errors:

Error: Connection timed out on request to localhost:50385
    at Timeout.timeoutHandler (/Users/luigi/timeout/node_modules/timed-out/index.js:10:11)
    at tryOnTimeout (timers.js:232:11)
    at Timer.listOnTimeout (timers.js:202:5)
Error: socket hang up
    at createHangUpError (_http_client.js:252:15)
    at Socket.socketCloseListener (_http_client.js:284:23)
    at emitOne (events.js:101:20)
    at Socket.emit (events.js:188:7)
    at TCP._handle.close [as _onclose] (net.js:493:12)

I wonder if it makes more sense to clear the timer when the connect event is emitted on the socket:

diff --git a/index.js b/index.js
index c3ff8b2..92c87a8 100644
--- a/index.js
+++ b/index.js
@@ -12,13 +12,16 @@ module.exports = function (req, time) {
        req.emit('error', e);
    }, time);

-   // Set additional timeout on socket - in case if remote
-   // server freeze after sending headers
-   req.setTimeout(time, function socketTimeoutHandler() {
-       req.abort();
-       var e = new Error('Socket timed out on request' + host);
-       e.code = 'ESOCKETTIMEDOUT';
-       req.emit('error', e);
+   req.on('socket', function assign(socket) {
+       socket.on('connect', function connect() {
+           clear();
+           socket.setTimeout(time, function socketTimeoutHandler() {
+               req.abort();
+               var e = new Error('Socket timed out on request' + host);
+               e.code = 'ESOCKETTIMEDOUT';
+               req.emit('error', e);
+           });
+       });
    });

    function clear() {
@@ -28,7 +31,5 @@ module.exports = function (req, time) {
        }
    }

-   return req
-       .on('response', clear)
-       .on('error', clear);
+   return req.on('error', clear);
 };

This makes timed-out emit a socket timeout error when the socket is connected but no data is received. The above example yields the following result:

Error: Socket timed out on request to localhost:50446
    at Socket.socketTimeoutHandler (/Users/luigi/timeout/node_modules/timed-out/index.js:20:13)
    at Socket.g (events.js:291:16)
    at emitNone (events.js:86:13)
    at Socket.emit (events.js:185:7)
    at Socket._onTimeout (net.js:334:8)
    at tryOnTimeout (timers.js:232:11)
    at Timer.listOnTimeout (timers.js:202:5)
Error: socket hang up
    at createHangUpError (_http_client.js:252:15)
    at Socket.socketCloseListener (_http_client.js:284:23)
    at emitOne (events.js:101:20)
    at Socket.emit (events.js:188:7)
    at TCP._handle.close [as _onclose] (net.js:493:12)

[floatdrop]

@lpinca yes, this makes perfect sense. Can you do a PR?

Only question is – is time in socket.setTimeout should be decreased by time, that socket event take?

[lpinca]

I'm not sure, as I see it, it should works like this:

• Wait for the connection event and if we don't get it in time emit the connection timeout error.
• Once the connection event is fired set the timeout on the socket.

There could indeed be a case where the connection is opened very late and then there will be no more activity on the socket. In this condition the error event will be fire after ~ 2 * time, but I think the same can also happens with the current implementation for example when the response event is fired very late then a chunk is sent and then there will be no more activity on the socket.

Anyway I think this is a breaking change because right now (correct me if I'm wrong) a request is aborted for example when streaming something that takes longer than time and a response is not sent by that time. With this change this is no longer true. Everything is fine as long as there activity on the socket and the connection has been established.

[lpinca]

I'll send a PR later and we can continue the discussion there, I guess.