Skip to content

Commit 4f39894

Browse files
fengmk2fengmk2
committed
bugfix: support agent.options.timeout on https agent
https on node 8, 10 won't set agent.options.timeout by default
1 parent 5c9f3bb commit 4f39894

File tree

3 files changed

+118
-15
lines changed

3 files changed

+118
-15
lines changed

‎lib/agent.js

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -144,15 +144,15 @@ class Agent extends OriginalAgent {
144144
super.reuseSocket(...args);
145145
const socket = args[0];
146146
const agentTimeout = this.options.timeout;
147-
if (socket.timeout !== agentTimeout) {
147+
if (getSocketTimeout(socket) !== agentTimeout) {
148148
// reset timeout before use
149149
socket.setTimeout(agentTimeout);
150150
debug('%s reset timeout to %sms', socket[SOCKET_NAME], agentTimeout);
151151
}
152152
socket[SOCKET_REQUEST_COUNT]++;
153153
debug('%s(requests: %s, finished: %s) reuse on addRequest, timeout %sms',
154154
socket[SOCKET_NAME], socket[SOCKET_REQUEST_COUNT], socket[SOCKET_REQUEST_FINISHED_COUNT],
155-
socket.timeout);
155+
getSocketTimeout(socket));
156156
}
157157

158158
[CREATE_ID]() {
@@ -162,6 +162,16 @@ class Agent extends OriginalAgent {
162162
}
163163

164164
[INIT_SOCKET](socket, options) {
165+
// bugfix here.
166+
// https on node 8, 10 won't set agent.options.timeout by default
167+
// TODO: need to fix on node itself
168+
if (options.timeout) {
169+
const timeout = getSocketTimeout(socket);
170+
if (!timeout) {
171+
socket.setTimeout(options.timeout);
172+
}
173+
}
174+
165175
if (this.keepAlive) {
166176
// Disable Nagle's algorithm: http://blog.caustik.com/2012/04/08/scaling-node-js-to-100k-concurrent-connections/
167177
// https://fengmk2.com/benchmark/nagle-algorithm-delayed-ack-mock.html
@@ -229,8 +239,14 @@ class Agent extends OriginalAgent {
229239
}
230240
}
231241

242+
// node 8 don't has timeout attribute on socket
243+
// https://github.com/nodejs/node/pull/21204/files#diff-e6ef024c3775d787c38487a6309e491dR408
244+
function getSocketTimeout(socket) {
245+
return socket.timeout || socket._idleTimeout;
246+
}
247+
232248
function installListeners(agent, socket, options) {
233-
debug('%s create, timeout %sms', socket[SOCKET_NAME], socket.timeout);
249+
debug('%s create, timeout %sms', socket[SOCKET_NAME], getSocketTimeout(socket));
234250

235251
// listener socket events: close, timeout, error, free
236252
function onFree() {
@@ -267,7 +283,7 @@ function installListeners(agent, socket, options) {
267283
const listenerCount = socket.listeners('timeout').length;
268284
debug('%s(requests: %s, finished: %s) timeout after %sms, listeners %s',
269285
socket[SOCKET_NAME], socket[SOCKET_REQUEST_COUNT], socket[SOCKET_REQUEST_FINISHED_COUNT],
270-
socket.timeout, listenerCount);
286+
getSocketTimeout(socket), listenerCount);
271287
agent.timeoutSocketCount++;
272288
const name = agent.getName(options);
273289
if (agent.freeSockets[name] && agent.freeSockets[name].indexOf(socket) !== -1) {
@@ -290,7 +306,7 @@ function installListeners(agent, socket, options) {
290306
if (listenerCount === 1) {
291307
const error = new Error('Socket timeout');
292308
error.code = 'ERR_SOCKET_TIMEOUT';
293-
error.timeout = socket.timeout;
309+
error.timeout = getSocketTimeout(socket);
294310
// must manually call socket.end() or socket.destroy() to end the connection.
295311
// https://nodejs.org/dist/latest-v10.x/docs/api/net.html#net_socket_settimeout_timeout_callback
296312
socket.destroy(error);

‎test/agent.test.js

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -317,20 +317,23 @@ describe('test/agent.test.js', () => {
317317
path: '/',
318318
}, res => {
319319
const socket1 = res.socket;
320-
assert(socket1.timeout === 1000);
320+
const timeout = socket1.timeout || socket1._idleTimeout;
321+
assert(timeout === 1000);
321322
assert(res.statusCode === 200);
322323
res.resume();
323324
res.on('end', () => {
324325
setImmediate(() => {
325-
assert(socket1.timeout === 1000);
326+
const timeout = socket1.timeout || socket1._idleTimeout;
327+
assert(timeout === 1000);
326328
http.get({
327329
agent,
328330
port,
329331
path: '/',
330332
}, res => {
331333
const socket2 = res.socket;
332334
assert(socket2 === socket1);
333-
assert(socket2.timeout === 1000);
335+
const timeout = socket2.timeout || socket2._idleTimeout;
336+
assert(timeout === 1000);
334337
assert(res.statusCode === 200);
335338
res.resume();
336339
res.on('end', done);
@@ -352,20 +355,23 @@ describe('test/agent.test.js', () => {
352355
path: '/?timeout=80',
353356
}, res => {
354357
const socket1 = res.socket;
355-
assert(socket1.timeout === 100);
358+
const timeout = socket1.timeout || socket1._idleTimeout;
359+
assert(timeout === 100);
356360
assert(res.statusCode === 200);
357361
res.resume();
358362
res.on('end', () => {
359363
setTimeout(() => {
360-
assert(socket1.timeout === 100);
364+
const timeout = socket1.timeout || socket1._idleTimeout;
365+
assert(timeout === 100);
361366
http.get({
362367
agent,
363368
port,
364369
path: '/',
365370
}, res => {
366371
const socket2 = res.socket;
367372
assert(socket2 === socket1);
368-
assert(socket2.timeout === 100);
373+
const timeout = socket2.timeout || socket2._idleTimeout;
374+
assert(timeout === 100);
369375
assert(res.statusCode === 200);
370376
res.resume();
371377
res.on('end', done);
@@ -422,7 +428,7 @@ describe('test/agent.test.js', () => {
422428
const agent = new Agent({
423429
timeout: 100,
424430
});
425-
http.get({
431+
const req = http.get({
426432
agent,
427433
port,
428434
path: '/hang',
@@ -431,6 +437,8 @@ describe('test/agent.test.js', () => {
431437
const originalException = process.listeners('uncaughtException').pop();
432438
process.removeListener('uncaughtException', originalException);
433439
process.once('uncaughtException', err => {
440+
// ignore future req error
441+
req.on('error', () => {});
434442
process.on('uncaughtException', originalException);
435443
assert(err);
436444
assert(err.message === 'Socket timeout');
@@ -1705,7 +1713,8 @@ describe('test/agent.test.js', () => {
17051713
res.resume();
17061714
res.on('end', () => {
17071715
setTimeout(function() {
1708-
assert(socket1.timeout <= 1000);
1716+
const timeout = socket1.timeout || socket1._idleTimeout;
1717+
assert(timeout <= 1000);
17091718
const req2 = http.get({
17101719
agent,
17111720
port,
@@ -1743,7 +1752,8 @@ describe('test/agent.test.js', () => {
17431752
res.resume();
17441753
res.on('end', () => {
17451754
setTimeout(function() {
1746-
assert(socket1.timeout === 500);
1755+
const timeout = socket1.timeout || socket1._idleTimeout;
1756+
assert(timeout === 500);
17471757
const req2 = http.get({
17481758
agent,
17491759
port,

‎test/https_agent.test.js

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const urlparse = require('url').parse;
55
const fs = require('fs');
66
const assert = require('assert');
77
const HttpsAgent = require('..').HttpsAgent;
8+
// const HttpsAgent = https.Agent;
89

910
describe('test/https_agent.test.js', () => {
1011
let app = null;
@@ -20,6 +21,7 @@ describe('test/https_agent.test.js', () => {
2021
key: fs.readFileSync(__dirname + '/fixtures/agenttest-key.pem'),
2122
cert: fs.readFileSync(__dirname + '/fixtures/agenttest-cert.pem'),
2223
}, (req, res) => {
24+
req.resume();
2325
if (req.url === '/error') {
2426
res.destroy();
2527
return;
@@ -30,9 +32,13 @@ describe('test/https_agent.test.js', () => {
3032
}
3133
const info = urlparse(req.url, true);
3234
if (info.query.timeout) {
35+
console.log('[new https request] %s %s, query %j', req.method, req.url, info.query);
3336
setTimeout(() => {
37+
res.writeHeader(200, {
38+
'Content-Length': `${info.query.timeout.length}`,
39+
});
3440
res.end(info.query.timeout);
35-
}, parseInt(info.query.timeout, 10));
41+
}, parseInt(info.query.timeout));
3642
return;
3743
}
3844
res.end(JSON.stringify({
@@ -75,6 +81,77 @@ describe('test/https_agent.test.js', () => {
7581
assert(Object.keys(agentkeepalive.freeSockets).length === 0);
7682
});
7783

84+
it('should req handle custom timeout error', done => {
85+
const req = https.get({
86+
agent: agentkeepalive,
87+
port,
88+
path: '/?timeout=100',
89+
ca: fs.readFileSync(__dirname + '/fixtures/ca.pem'),
90+
timeout: 50,
91+
}, res => {
92+
console.log(res.statusCode, res.headers);
93+
res.resume();
94+
res.on('end', () => {
95+
done(new Error('should not run this'));
96+
});
97+
}).on('error', err => {
98+
assert(err);
99+
assert(err.message === 'socket hang up');
100+
done();
101+
});
102+
103+
// node 8 don't support options.timeout on http.get
104+
if (process.version.startsWith('v8.')) {
105+
req.setTimeout(50);
106+
}
107+
req.on('timeout', () => {
108+
req.abort();
109+
});
110+
});
111+
112+
it('should agent handle default timeout error [bugfix for node 8, 10]', done => {
113+
const agent = new HttpsAgent({
114+
freeSocketTimeout: 1000,
115+
timeout: 50,
116+
maxSockets: 5,
117+
maxFreeSockets: 5,
118+
});
119+
https.get({
120+
agent,
121+
port,
122+
path: '/?timeout=100',
123+
ca: fs.readFileSync(__dirname + '/fixtures/ca.pem'),
124+
}, res => {
125+
console.log(res.statusCode, res.headers);
126+
res.resume();
127+
res.on('end', () => {
128+
done(new Error('should not run this'));
129+
});
130+
}).on('error', err => {
131+
assert(err);
132+
assert(err.message === 'Socket timeout');
133+
done();
134+
});
135+
});
136+
137+
it('should don\'t set timeout on options.timeout = 0', done => {
138+
const agent = new HttpsAgent({
139+
freeSocketTimeout: 1000,
140+
timeout: 0,
141+
maxSockets: 5,
142+
maxFreeSockets: 5,
143+
});
144+
https.get({
145+
agent,
146+
port,
147+
path: '/',
148+
ca: fs.readFileSync(__dirname + '/fixtures/ca.pem'),
149+
}, res => {
150+
res.resume();
151+
res.on('end', done);
152+
});
153+
});
154+
78155
it('should free socket timeout', done => {
79156
https.get({
80157
agent: agentkeepalive,

0 commit comments

Comments
 (0)