Skip to content

Commit

Permalink
simplify code (move out socket timeout part)
Browse files Browse the repository at this point in the history
  • Loading branch information
floatdrop committed Jan 16, 2017
1 parent e216d30 commit 42f6998
Showing 1 changed file with 13 additions and 17 deletions.
30 changes: 13 additions & 17 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,27 @@ module.exports = function (req, time) {
}, delays.connect);
}

if (delays.socket !== undefined) {
// Abort the request if there is no activity on the socket for more
// than `delays.socket` milliseconds.
req.setTimeout(delays.socket, function socketTimeoutHandler() {
req.abort();
var e = new Error('Socket timed out on request' + host);
e.code = 'ESOCKETTIMEDOUT';
req.emit('error', e);
});
}

// Clear the connection timeout timer once a socket is assigned to the
// request and is connected.
req.on('socket', function assign(socket) {
// Socket may come from Agent pool and may be already connected.
if (!(socket.connecting || socket._connecting)) {
connect();
clear();
return;
}

socket.once('connect', connect);
socket.once('connect', clear);
});

function clear() {
Expand All @@ -36,20 +47,5 @@ module.exports = function (req, time) {
}
}

function connect() {
clear();

if (delays.socket !== undefined) {
// Abort the request if there is no activity on the socket for more
// than `delays.socket` milliseconds.
req.setTimeout(delays.socket, function socketTimeoutHandler() {
req.abort();
var e = new Error('Socket timed out on request' + host);
e.code = 'ESOCKETTIMEDOUT';
req.emit('error', e);
});
}
}

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

2 comments on commit 42f6998

@lpinca
Copy link
Contributor

@lpinca lpinca commented on 42f6998 Jan 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@floatdrop sorry to pester.
This slightly changes the behavior. req.setTimeout() sets the socket timeout as soon as a socket is assigned to the request, before the socket is connected, despite the documentation saying otherwise.
With this change an ESOCKETTIMEDOUT error could be incorrectly raised even if the socket fails to connect. See nodejs/node#8895.

@floatdrop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpinca thanks for noticing!

Please sign in to comment.