GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http: fix socket re-use races · nodejs/node@f9123eb

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • doc/api
    • http.md
  • lib
    • _http_agent.js
  • test/parallel
    • test-http-agent-timeout-option.js
    • test-http-agent-timeout.js
    • test-http-client-set-timeout-after-end.js
    • test-http-client-set-timeout.js
    • test-http-client-timeout-option-listeners.js
    • test-http-client-timeout-option-with-agent.js
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎doc/api/http.md‎

Copy file name to clipboardExpand all lines: doc/api/http.md
+3Lines changed: 3 additions & 0 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,9 @@ added: v0.11.4
239239
An object which contains arrays of sockets currently awaiting use by
240240
the agent when `keepAlive` is enabled. Do not modify.
241241

242+
Sockets in the `freeSockets` list will be automatically destroyed and
243+
removed from the array on `'timeout'`.
244+
242245
### `agent.getName(options)`
243246
<!-- YAML
244247
added: v0.11.4
Collapse file

‎lib/_http_agent.js‎

Copy file name to clipboardExpand all lines: lib/_http_agent.js
+34-16Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,12 @@ function Agent(options) {
115115
socket[async_id_symbol] = -1;
116116
socket._httpMessage = null;
117117
this.removeSocket(socket, options);
118+
119+
const agentTimeout = this.options.timeout || 0;
120+
if (socket.timeout !== agentTimeout) {
121+
socket.setTimeout(agentTimeout);
122+
}
123+
118124
freeSockets.push(socket);
119125
} else {
120126
// Implementation doesn't want to keep socket alive
@@ -197,12 +203,21 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
197203
this.sockets[name] = [];
198204
}
199205

200-
const freeLen = this.freeSockets[name] ? this.freeSockets[name].length : 0;
206+
const freeSockets = this.freeSockets[name];
207+
let socket;
208+
if (freeSockets) {
209+
while (freeSockets.length && freeSockets[0].destroyed) {
210+
freeSockets.shift();
211+
}
212+
socket = freeSockets.shift();
213+
if (!freeSockets.length)
214+
delete this.freeSockets[name];
215+
}
216+
217+
const freeLen = freeSockets ? freeSockets.length : 0;
201218
const sockLen = freeLen + this.sockets[name].length;
202219

203-
if (freeLen) {
204-
// We have a free socket, so use that.
205-
const socket = this.freeSockets[name].shift();
220+
if (socket) {
206221
// Guard against an uninitialized or user supplied Socket.
207222
const handle = socket._handle;
208223
if (handle && typeof handle.asyncReset === 'function') {
@@ -211,10 +226,6 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
211226
socket[async_id_symbol] = handle.getAsyncId();
212227
}
213228

214-
// don't leak
215-
if (!this.freeSockets[name].length)
216-
delete this.freeSockets[name];
217-
218229
this.reuseSocket(socket, req);
219230
setRequestSocket(this, req, socket);
220231
this.sockets[name].push(socket);
@@ -309,6 +320,20 @@ function installListeners(agent, s, options) {
309320
}
310321
s.on('close', onClose);
311322

323+
function onTimeout() {
324+
debug('CLIENT socket onTimeout');
325+
326+
// Destroy if in free list.
327+
// TODO(ronag): Always destroy, even if not in free list.
328+
const sockets = agent.freeSockets;
329+
for (const name of ObjectKeys(sockets)) {
330+
if (sockets[name].includes(s)) {
331+
return s.destroy();
332+
}
333+
}
334+
}
335+
s.on('timeout', onTimeout);
336+
312337
function onRemove() {
313338
// We need this function for cases like HTTP 'upgrade'
314339
// (defined by WebSockets) where we need to remove a socket from the
@@ -317,6 +342,7 @@ function installListeners(agent, s, options) {
317342
agent.removeSocket(s, options);
318343
s.removeListener('close', onClose);
319344
s.removeListener('free', onFree);
345+
s.removeListener('timeout', onTimeout);
320346
s.removeListener('agentRemove', onRemove);
321347
}
322348
s.on('agentRemove', onRemove);
@@ -399,14 +425,6 @@ function setRequestSocket(agent, req, socket) {
399425
return;
400426
}
401427
socket.setTimeout(req.timeout);
402-
// Reset timeout after response end
403-
req.once('response', (res) => {
404-
res.once('end', () => {
405-
if (socket.timeout !== agentTimeout) {
406-
socket.setTimeout(agentTimeout);
407-
}
408-
});
409-
});
410428
}
411429

412430
function emitErrorNT(emitter, err) {
Collapse file

‎test/parallel/test-http-agent-timeout-option.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-agent-timeout-option.js
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => {
1818

1919
const listeners = socket.listeners('timeout');
2020

21-
strictEqual(listeners.length, 1);
22-
strictEqual(listeners[0], request.timeoutCb);
21+
strictEqual(listeners.length, 2);
22+
strictEqual(listeners[1], request.timeoutCb);
2323
}));
Collapse file

‎test/parallel/test-http-agent-timeout.js‎

Copy file name to clipboard
+94Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const http = require('http');
6+
7+
{
8+
// Ensure reuse of successful sockets.
9+
10+
const agent = new http.Agent({ keepAlive: true });
11+
12+
const server = http.createServer((req, res) => {
13+
res.end();
14+
});
15+
16+
server.listen(0, common.mustCall(() => {
17+
let socket;
18+
http.get({ port: server.address().port, agent })
19+
.on('response', common.mustCall((res) => {
20+
socket = res.socket;
21+
assert(socket);
22+
res.resume();
23+
socket.on('free', common.mustCall(() => {
24+
http.get({ port: server.address().port, agent })
25+
.on('response', common.mustCall((res) => {
26+
assert.strictEqual(socket, res.socket);
27+
assert(socket);
28+
agent.destroy();
29+
server.close();
30+
}));
31+
}));
32+
}));
33+
}));
34+
}
35+
36+
{
37+
// Ensure that timeouted sockets are not reused.
38+
39+
const agent = new http.Agent({ keepAlive: true, timeout: 50 });
40+
41+
const server = http.createServer((req, res) => {
42+
res.end();
43+
});
44+
45+
server.listen(0, common.mustCall(() => {
46+
http.get({ port: server.address().port, agent })
47+
.on('response', common.mustCall((res) => {
48+
const socket = res.socket;
49+
assert(socket);
50+
res.resume();
51+
socket.on('free', common.mustCall(() => {
52+
socket.on('timeout', common.mustCall(() => {
53+
http.get({ port: server.address().port, agent })
54+
.on('response', common.mustCall((res) => {
55+
assert.notStrictEqual(socket, res.socket);
56+
assert.strictEqual(socket.destroyed, true);
57+
agent.destroy();
58+
server.close();
59+
}));
60+
}));
61+
}));
62+
}));
63+
}));
64+
}
65+
66+
{
67+
// Ensure that destroyed sockets are not reused.
68+
69+
const agent = new http.Agent({ keepAlive: true });
70+
71+
const server = http.createServer((req, res) => {
72+
res.end();
73+
});
74+
75+
server.listen(0, common.mustCall(() => {
76+
let socket;
77+
http.get({ port: server.address().port, agent })
78+
.on('response', common.mustCall((res) => {
79+
socket = res.socket;
80+
assert(socket);
81+
res.resume();
82+
socket.on('free', common.mustCall(() => {
83+
socket.destroy();
84+
http.get({ port: server.address().port, agent })
85+
.on('response', common.mustCall((res) => {
86+
assert.notStrictEqual(socket, res.socket);
87+
assert(socket);
88+
agent.destroy();
89+
server.close();
90+
}));
91+
}));
92+
}));
93+
}));
94+
}
Collapse file

‎test/parallel/test-http-client-set-timeout-after-end.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-client-set-timeout-after-end.js
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ server.listen(0, () => {
2020
const req = get({ agent, port }, (res) => {
2121
res.on('end', () => {
2222
strictEqual(req.setTimeout(0), req);
23-
strictEqual(socket.listenerCount('timeout'), 0);
23+
strictEqual(socket.listenerCount('timeout'), 1);
2424
agent.destroy();
2525
server.close();
2626
});
Collapse file

‎test/parallel/test-http-client-set-timeout.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-client-set-timeout.js
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ server.listen(0, mustCall(() => {
4242
}));
4343

4444
req.on('timeout', mustCall(() => {
45-
strictEqual(req.socket.listenerCount('timeout'), 0);
45+
strictEqual(req.socket.listenerCount('timeout'), 1);
4646
req.destroy();
4747
}));
4848
}));
Collapse file

‎test/parallel/test-http-client-timeout-option-listeners.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-client-timeout-option-listeners.js
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ const options = {
2424
server.listen(0, options.host, common.mustCall(() => {
2525
options.port = server.address().port;
2626
doRequest(common.mustCall((numListeners) => {
27-
assert.strictEqual(numListeners, 1);
27+
assert.strictEqual(numListeners, 2);
2828
doRequest(common.mustCall((numListeners) => {
29-
assert.strictEqual(numListeners, 1);
29+
assert.strictEqual(numListeners, 2);
3030
server.close();
3131
agent.destroy();
3232
}));
Collapse file

‎test/parallel/test-http-client-timeout-option-with-agent.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-client-timeout-option-with-agent.js
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@ request.on('socket', mustCall((socket) => {
1818

1919
const listeners = socket.listeners('timeout');
2020

21-
strictEqual(listeners.length, 1);
22-
strictEqual(listeners[0], request.timeoutCb);
21+
strictEqual(listeners.length, 2);
22+
strictEqual(listeners[1], request.timeoutCb);
2323
}));

0 commit comments

Comments
0 (0)
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.