Skip to content

Commit

Permalink
bugfix: support agent.options.timeout on https agent
Browse files Browse the repository at this point in the history 
https on node 8, 10 won't set agent.options.timeout by default
  • Loading branch information
@fengmk2
fengmk2 committed Oct 22, 2018
1 parent 5c9f3bb commit 4f39894
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 15 deletions.
26 changes: 21 additions & 5 deletions lib/agent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,15 +144,15 @@ class Agent extends OriginalAgent {
super.reuseSocket(...args);
const socket = args[0];
const agentTimeout = this.options.timeout;
if (socket.timeout !== agentTimeout) {
if (getSocketTimeout(socket) !== agentTimeout) {
// reset timeout before use
socket.setTimeout(agentTimeout);
debug('%s reset timeout to %sms', socket[SOCKET_NAME], agentTimeout);
}
socket[SOCKET_REQUEST_COUNT]++;
debug('%s(requests: %s, finished: %s) reuse on addRequest, timeout %sms',
socket[SOCKET_NAME], socket[SOCKET_REQUEST_COUNT], socket[SOCKET_REQUEST_FINISHED_COUNT],
socket.timeout);
getSocketTimeout(socket));
}

[CREATE_ID]() {
Expand All @@ -162,6 +162,16 @@ class Agent extends OriginalAgent {
}

[INIT_SOCKET](socket, options) {
// bugfix here.
// https on node 8, 10 won't set agent.options.timeout by default
// TODO: need to fix on node itself
if (options.timeout) {
const timeout = getSocketTimeout(socket);
if (!timeout) {
socket.setTimeout(options.timeout);
}
}

if (this.keepAlive) {
// Disable Nagle's algorithm: http://blog.caustik.com/2012/04/08/scaling-node-js-to-100k-concurrent-connections/
// https://fengmk2.com/benchmark/nagle-algorithm-delayed-ack-mock.html
Expand Down Expand Up @@ -229,8 +239,14 @@ class Agent extends OriginalAgent {
}
}

// node 8 don't has timeout attribute on socket
// https://github.com/nodejs/node/pull/21204/files#diff-e6ef024c3775d787c38487a6309e491dR408
function getSocketTimeout(socket) {
return socket.timeout || socket._idleTimeout;
}

function installListeners(agent, socket, options) {
debug('%s create, timeout %sms', socket[SOCKET_NAME], socket.timeout);
debug('%s create, timeout %sms', socket[SOCKET_NAME], getSocketTimeout(socket));

// listener socket events: close, timeout, error, free
function onFree() {
Expand Down Expand Up @@ -267,7 +283,7 @@ function installListeners(agent, socket, options) {
const listenerCount = socket.listeners('timeout').length;
debug('%s(requests: %s, finished: %s) timeout after %sms, listeners %s',
socket[SOCKET_NAME], socket[SOCKET_REQUEST_COUNT], socket[SOCKET_REQUEST_FINISHED_COUNT],
socket.timeout, listenerCount);
getSocketTimeout(socket), listenerCount);
agent.timeoutSocketCount++;
const name = agent.getName(options);
if (agent.freeSockets[name] && agent.freeSockets[name].indexOf(socket) !== -1) {
Expand All @@ -290,7 +306,7 @@ function installListeners(agent, socket, options) {
if (listenerCount === 1) {
const error = new Error('Socket timeout');
error.code = 'ERR_SOCKET_TIMEOUT';
error.timeout = socket.timeout;
error.timeout = getSocketTimeout(socket);
// must manually call socket.end() or socket.destroy() to end the connection.
// https://nodejs.org/dist/latest-v10.x/docs/api/net.html#net_socket_settimeout_timeout_callback
socket.destroy(error);
Expand Down
28 changes: 19 additions & 9 deletions test/agent.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,20 +317,23 @@ describe('test/agent.test.js', () => {
path: '/',
}, res => {
const socket1 = res.socket;
assert(socket1.timeout === 1000);
const timeout = socket1.timeout || socket1._idleTimeout;
assert(timeout === 1000);
assert(res.statusCode === 200);
res.resume();
res.on('end', () => {
setImmediate(() => {
assert(socket1.timeout === 1000);
const timeout = socket1.timeout || socket1._idleTimeout;
assert(timeout === 1000);
http.get({
agent,
port,
path: '/',
}, res => {
const socket2 = res.socket;
assert(socket2 === socket1);
assert(socket2.timeout === 1000);
const timeout = socket2.timeout || socket2._idleTimeout;
assert(timeout === 1000);
assert(res.statusCode === 200);
res.resume();
res.on('end', done);
Expand All @@ -352,20 +355,23 @@ describe('test/agent.test.js', () => {
path: '/?timeout=80',
}, res => {
const socket1 = res.socket;
assert(socket1.timeout === 100);
const timeout = socket1.timeout || socket1._idleTimeout;
assert(timeout === 100);
assert(res.statusCode === 200);
res.resume();
res.on('end', () => {
setTimeout(() => {
assert(socket1.timeout === 100);
const timeout = socket1.timeout || socket1._idleTimeout;
assert(timeout === 100);
http.get({
agent,
port,
path: '/',
}, res => {
const socket2 = res.socket;
assert(socket2 === socket1);
assert(socket2.timeout === 100);
const timeout = socket2.timeout || socket2._idleTimeout;
assert(timeout === 100);
assert(res.statusCode === 200);
res.resume();
res.on('end', done);
Expand Down Expand Up @@ -422,7 +428,7 @@ describe('test/agent.test.js', () => {
const agent = new Agent({
timeout: 100,
});
http.get({
const req = http.get({
agent,
port,
path: '/hang',
Expand All @@ -431,6 +437,8 @@ describe('test/agent.test.js', () => {
const originalException = process.listeners('uncaughtException').pop();
process.removeListener('uncaughtException', originalException);
process.once('uncaughtException', err => {
// ignore future req error
req.on('error', () => {});
process.on('uncaughtException', originalException);
assert(err);
assert(err.message === 'Socket timeout');
Expand Down Expand Up @@ -1705,7 +1713,8 @@ describe('test/agent.test.js', () => {
res.resume();
res.on('end', () => {
setTimeout(function() {
assert(socket1.timeout <= 1000);
const timeout = socket1.timeout || socket1._idleTimeout;
assert(timeout <= 1000);
const req2 = http.get({
agent,
port,
Expand Down Expand Up @@ -1743,7 +1752,8 @@ describe('test/agent.test.js', () => {
res.resume();
res.on('end', () => {
setTimeout(function() {
assert(socket1.timeout === 500);
const timeout = socket1.timeout || socket1._idleTimeout;
assert(timeout === 500);
const req2 = http.get({
agent,
port,
Expand Down
79 changes: 78 additions & 1 deletion test/https_agent.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const urlparse = require('url').parse;
const fs = require('fs');
const assert = require('assert');
const HttpsAgent = require('..').HttpsAgent;
// const HttpsAgent = https.Agent;

describe('test/https_agent.test.js', () => {
let app = null;
Expand All @@ -20,6 +21,7 @@ describe('test/https_agent.test.js', () => {
key: fs.readFileSync(__dirname + '/fixtures/agenttest-key.pem'),
cert: fs.readFileSync(__dirname + '/fixtures/agenttest-cert.pem'),
}, (req, res) => {
req.resume();
if (req.url === '/error') {
res.destroy();
return;
Expand All @@ -30,9 +32,13 @@ describe('test/https_agent.test.js', () => {
}
const info = urlparse(req.url, true);
if (info.query.timeout) {
console.log('[new https request] %s %s, query %j', req.method, req.url, info.query);
setTimeout(() => {
res.writeHeader(200, {
'Content-Length': `${info.query.timeout.length}`,
});
res.end(info.query.timeout);
}, parseInt(info.query.timeout, 10));
}, parseInt(info.query.timeout));
return;
}
res.end(JSON.stringify({
Expand Down Expand Up @@ -75,6 +81,77 @@ describe('test/https_agent.test.js', () => {
assert(Object.keys(agentkeepalive.freeSockets).length === 0);
});

it('should req handle custom timeout error', done => {
const req = https.get({
agent: agentkeepalive,
port,
path: '/?timeout=100',
ca: fs.readFileSync(__dirname + '/fixtures/ca.pem'),
timeout: 50,
}, res => {
console.log(res.statusCode, res.headers);
res.resume();
res.on('end', () => {
done(new Error('should not run this'));
});
}).on('error', err => {
assert(err);
assert(err.message === 'socket hang up');
done();
});

// node 8 don't support options.timeout on http.get
if (process.version.startsWith('v8.')) {
req.setTimeout(50);
}
req.on('timeout', () => {
req.abort();
});
});

it('should agent handle default timeout error [bugfix for node 8, 10]', done => {
const agent = new HttpsAgent({
freeSocketTimeout: 1000,
timeout: 50,
maxSockets: 5,
maxFreeSockets: 5,
});
https.get({
agent,
port,
path: '/?timeout=100',
ca: fs.readFileSync(__dirname + '/fixtures/ca.pem'),
}, res => {
console.log(res.statusCode, res.headers);
res.resume();
res.on('end', () => {
done(new Error('should not run this'));
});
}).on('error', err => {
assert(err);
assert(err.message === 'Socket timeout');
done();
});
});

it('should don\'t set timeout on options.timeout = 0', done => {
const agent = new HttpsAgent({
freeSocketTimeout: 1000,
timeout: 0,
maxSockets: 5,
maxFreeSockets: 5,
});
https.get({
agent,
port,
path: '/',
ca: fs.readFileSync(__dirname + '/fixtures/ca.pem'),
}, res => {
res.resume();
res.on('end', done);
});
});

it('should free socket timeout', done => {
https.get({
agent: agentkeepalive,
Expand Down

0 comments on commit 4f39894

Please sign in to comment.