Skip to content

Commit a12dfcb

Browse files
committed
http: respect Keep-Alive timeout hint when agent has no explicit timeout
When http.Agent is created with keepAlive: true and no explicit timeout, agentTimeout defaults to 0. The condition serverHintTimeout < agentTimeout is never true when agentTimeout === 0, so socket.setTimeout(0) is called, disabling the idle timer and leaving stale sockets in freeSockets. Fix by also applying the server hint when agentTimeout === 0. Fixes: #66734 Refs: #52649 Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>
1 parent 1c90519 commit a12dfcb

2 files changed

Lines changed: 163 additions & 1 deletion

File tree

‎lib/internal/http/agent.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -654,7 +654,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
654654
// Cannot safely reuse the socket because the server timeout is
655655
// too short
656656
canKeepSocketAlive = false;
657-
} else if (serverHintTimeout < agentTimeout) {
657+
} else if (agentTimeout === 0 || serverHintTimeout < agentTimeout) {
658658
agentTimeout = serverHintTimeout;
659659
}
660660
}
Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
'use strict';
2+
3+
// Regression test for https://github.com/nodejs/node/issues/66734
4+
//
5+
// When http.Agent is created with keepAlive: true but no explicit timeout,
6+
// the server's Keep-Alive: timeout=N hint must be applied to the idle socket.
7+
// Previously, agentTimeout defaulted to 0 and the condition
8+
// `serverHintTimeout < agentTimeout` was never true, so setTimeout(0) was
9+
// called, disabling the timer and leaving stale sockets in freeSockets.
10+
11+
const common = require('../common');
12+
const assert = require('assert');
13+
const http = require('http');
14+
15+
// Helper: returns the first idle socket for the agent, after the 'free'
16+
// event has been processed (one setImmediate tick after response end).
17+
function getIdleSocket(agent) {
18+
const key = Object.keys(agent.freeSockets)[0];
19+
return key ? agent.freeSockets[key][0] : undefined;
20+
}
21+
22+
// Case 1: Agent has no explicit timeout.
23+
// Server sends Keep-Alive: timeout=5.
24+
// Expected: socket.timeout === 5000 - 1000 (default buffer) === 4000.
25+
{
26+
const SERVER_HINT_S = 5;
27+
const EXPECTED_TIMEOUT = (SERVER_HINT_S * 1000) - 1000; // 4000ms
28+
29+
const server = http.createServer(common.mustCall((req, res) => {
30+
res.writeHead(200, { 'Keep-Alive': `timeout=${SERVER_HINT_S}` });
31+
res.end('ok');
32+
}));
33+
34+
server.listen(0, common.mustCall(() => {
35+
const agent = new http.Agent({ keepAlive: true });
36+
37+
http.get({ port: server.address().port, agent }, common.mustCall((res) => {
38+
res.resume();
39+
res.on('end', common.mustCall(() => {
40+
setImmediate(common.mustCall(() => {
41+
const socket = getIdleSocket(agent);
42+
assert.ok(socket, 'socket should be in freeSockets');
43+
assert.strictEqual(
44+
socket.timeout,
45+
EXPECTED_TIMEOUT,
46+
`Case 1: expected socket.timeout ${EXPECTED_TIMEOUT}, ` +
47+
`got ${socket.timeout}`
48+
);
49+
agent.destroy();
50+
server.close();
51+
}));
52+
}));
53+
}));
54+
}));
55+
}
56+
57+
// Case 2: Agent has an explicit timeout longer than the server hint.
58+
// Server sends Keep-Alive: timeout=5 (4000ms after buffer).
59+
// Agent timeout is 10000ms.
60+
// Expected: server hint wins → socket.timeout === 4000.
61+
{
62+
const SERVER_HINT_S = 5;
63+
const AGENT_TIMEOUT = 10000;
64+
const EXPECTED_TIMEOUT = (SERVER_HINT_S * 1000) - 1000; // 4000ms
65+
66+
const server = http.createServer(common.mustCall((req, res) => {
67+
res.writeHead(200, { 'Keep-Alive': `timeout=${SERVER_HINT_S}` });
68+
res.end('ok');
69+
}));
70+
71+
server.listen(0, common.mustCall(() => {
72+
const agent = new http.Agent({ keepAlive: true, timeout: AGENT_TIMEOUT });
73+
74+
http.get({ port: server.address().port, agent }, common.mustCall((res) => {
75+
res.resume();
76+
res.on('end', common.mustCall(() => {
77+
setImmediate(common.mustCall(() => {
78+
const socket = getIdleSocket(agent);
79+
assert.ok(socket, 'socket should be in freeSockets');
80+
assert.strictEqual(
81+
socket.timeout,
82+
EXPECTED_TIMEOUT,
83+
`Case 2: expected socket.timeout ${EXPECTED_TIMEOUT}, ` +
84+
`got ${socket.timeout}`
85+
);
86+
agent.destroy();
87+
server.close();
88+
}));
89+
}));
90+
}));
91+
}));
92+
}
93+
94+
// Case 3: Agent has an explicit timeout shorter than the server hint.
95+
// Server sends Keep-Alive: timeout=10 (9000ms after buffer).
96+
// Agent timeout is 3000ms.
97+
// Expected: agent timeout wins → socket.timeout === 3000.
98+
{
99+
const SERVER_HINT_S = 10;
100+
const AGENT_TIMEOUT = 3000;
101+
const EXPECTED_TIMEOUT = AGENT_TIMEOUT; // 3000ms
102+
103+
const server = http.createServer(common.mustCall((req, res) => {
104+
res.writeHead(200, { 'Keep-Alive': `timeout=${SERVER_HINT_S}` });
105+
res.end('ok');
106+
}));
107+
108+
server.listen(0, common.mustCall(() => {
109+
const agent = new http.Agent({ keepAlive: true, timeout: AGENT_TIMEOUT });
110+
111+
http.get({ port: server.address().port, agent }, common.mustCall((res) => {
112+
res.resume();
113+
res.on('end', common.mustCall(() => {
114+
setImmediate(common.mustCall(() => {
115+
const socket = getIdleSocket(agent);
116+
assert.ok(socket, 'socket should be in freeSockets');
117+
assert.strictEqual(
118+
socket.timeout,
119+
EXPECTED_TIMEOUT,
120+
`Case 3: expected socket.timeout ${EXPECTED_TIMEOUT}, ` +
121+
`got ${socket.timeout}`
122+
);
123+
agent.destroy();
124+
server.close();
125+
}));
126+
}));
127+
}));
128+
}));
129+
}
130+
131+
// Case 4: Server sends no Keep-Alive hint (keepAliveTimeout: 0 suppresses the
132+
// automatic header), no agent timeout.
133+
// Expected: socket.timeout === 0 (no timer set — existing behaviour unchanged).
134+
{
135+
const server = http.createServer(
136+
{ keepAliveTimeout: 0 },
137+
common.mustCall((req, res) => {
138+
res.end('ok');
139+
})
140+
);
141+
142+
server.listen(0, common.mustCall(() => {
143+
const agent = new http.Agent({ keepAlive: true });
144+
145+
http.get({ port: server.address().port, agent }, common.mustCall((res) => {
146+
res.resume();
147+
res.on('end', common.mustCall(() => {
148+
setImmediate(common.mustCall(() => {
149+
const socket = getIdleSocket(agent);
150+
assert.ok(socket, 'socket should be in freeSockets');
151+
assert.strictEqual(
152+
socket.timeout,
153+
0,
154+
`Case 4: expected socket.timeout 0, got ${socket.timeout}`
155+
);
156+
agent.destroy();
157+
server.close();
158+
}));
159+
}));
160+
}));
161+
}));
162+
}

0 commit comments

Comments
 (0)