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 keep-alive socket reuse race in requestOnFinish · nodejs/node@ce0ebd8

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • lib
    • _http_client.js
  • test/parallel
    • test-http-expect-continue-reuse-race.js
    • test-https-expect-continue-reuse-race.js
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/_http_client.js‎

Copy file name to clipboardExpand all lines: lib/_http_client.js
+5-1Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -870,7 +870,11 @@ function responseOnTimeout() {
870870
function requestOnFinish() {
871871
const req = this;
872872

873-
if (req.shouldKeepAlive && req._ended)
873+
// If the response ends before this request finishes writing, `responseOnEnd()`
874+
// already released the socket. When `finish` fires later, that socket may
875+
// belong to a different request, so only call `responseKeepAlive()` when the
876+
// original request is still alive (`!req.destroyed`).
877+
if (req.shouldKeepAlive && req._ended && !req.destroyed)
874878
responseKeepAlive(req);
875879
}
876880

Collapse file

‎test/parallel/test-http-expect-continue-reuse-race.js‎

Copy file name to clipboard
+117Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
'use strict';
2+
3+
// Regression test for a keep-alive socket reuse race condition.
4+
//
5+
// The race is between responseOnEnd() and requestOnFinish(), both of which
6+
// can call responseKeepAlive(). The window is: req.end() has been called,
7+
// the socket write has completed (writableFinished true), but the write
8+
// callback that emits the 'finish' event has not fired yet.
9+
//
10+
// With plain HTTP the window is normally too narrow to hit. This test
11+
// widens it by delaying every client-socket write *callback* by a few
12+
// milliseconds (the actual I/O is not delayed, so writableFinished becomes
13+
// true while the 'finish'-emitting callback is still pending).
14+
//
15+
// With Expect: 100-continue, the server responds quickly while the client
16+
// delays req.end() just slightly (setTimeout 0), creating the perfect
17+
// timing for the response to arrive in that window.
18+
//
19+
// On unpatched Node, the double responseKeepAlive() call corrupts the
20+
// socket by stripping a subsequent request's listeners and emitting a
21+
// spurious 'free' event, causing requests to hang / time out.
22+
23+
const common = require('../common');
24+
const assert = require('assert');
25+
const http = require('http');
26+
27+
const REQUEST_COUNT = 100;
28+
const agent = new http.Agent({ keepAlive: true, maxSockets: 1 });
29+
30+
// Delay every write *callback* on the client socket so that
31+
// socket.writableLength drops to 0 (writableFinished becomes true) before
32+
// the callback that ultimately emits the 'finish' event fires. With
33+
// HTTPS the TLS layer provides this gap naturally; for plain HTTP we
34+
// need to create it artificially.
35+
const patchedSockets = new WeakSet();
36+
function patchSocket(socket) {
37+
if (patchedSockets.has(socket)) return;
38+
patchedSockets.add(socket);
39+
const delay = 5;
40+
const origWrite = socket.write;
41+
socket.write = function(chunk, encoding, cb) {
42+
if (typeof encoding === 'function') {
43+
cb = encoding;
44+
encoding = null;
45+
}
46+
if (typeof cb === 'function') {
47+
const orig = cb;
48+
cb = (...args) => setTimeout(() => orig(...args), delay);
49+
}
50+
return origWrite.call(this, chunk, encoding, cb);
51+
};
52+
}
53+
54+
const server = http.createServer(common.mustCall((req, res) => {
55+
req.on('error', common.mustNotCall());
56+
res.writeHead(200);
57+
res.end();
58+
}, REQUEST_COUNT));
59+
60+
server.listen(0, common.mustCall(() => {
61+
const { port } = server.address();
62+
63+
async function run() {
64+
try {
65+
for (let i = 0; i < REQUEST_COUNT; i++) {
66+
await sendRequest(port);
67+
}
68+
} finally {
69+
agent.destroy();
70+
server.close();
71+
}
72+
}
73+
74+
run().then(common.mustCall());
75+
}));
76+
77+
function sendRequest(port) {
78+
let timeout;
79+
const promise = new Promise((resolve, reject) => {
80+
function done(err) {
81+
clearTimeout(timeout);
82+
if (err)
83+
reject(err);
84+
else
85+
resolve();
86+
}
87+
88+
const req = http.request({
89+
port,
90+
host: '127.0.0.1',
91+
method: 'POST',
92+
agent,
93+
headers: {
94+
'Content-Length': '0',
95+
'Expect': '100-continue',
96+
},
97+
}, common.mustCall((res) => {
98+
assert.strictEqual(res.statusCode, 200);
99+
res.resume();
100+
res.once('end', done);
101+
res.once('error', done);
102+
}));
103+
104+
req.on('socket', patchSocket);
105+
106+
timeout = setTimeout(() => {
107+
const err = new Error('request timed out');
108+
req.destroy(err);
109+
done(err);
110+
}, common.platformTimeout(5000));
111+
112+
req.once('error', done);
113+
114+
setTimeout(() => req.end(Buffer.alloc(0)), 0);
115+
});
116+
return promise.finally(() => clearTimeout(timeout));
117+
}
Collapse file

‎test/parallel/test-https-expect-continue-reuse-race.js‎

Copy file name to clipboard
+97Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
'use strict';
2+
3+
// Regression test for a keep-alive socket reuse race condition.
4+
//
5+
// The race is between responseOnEnd() and requestOnFinish(), both of which
6+
// can call responseKeepAlive(). The window is: req.end() has been called,
7+
// the socket write has completed (writableFinished true), but the write
8+
// callback that emits the 'finish' event has not fired yet.
9+
//
10+
// HTTPS widens this window because the TLS layer introduces async
11+
// indirection between the actual write completion and the JS callback.
12+
//
13+
// With Expect: 100-continue, the server responds quickly while the client
14+
// delays req.end() just slightly (setTimeout 0), creating the perfect
15+
// timing for the response to arrive in that window.
16+
//
17+
// On unpatched Node, the double responseKeepAlive() call corrupts the
18+
// socket by stripping a subsequent request's listeners and emitting a
19+
// spurious 'free' event, causing requests to hang / time out.
20+
21+
const common = require('../common');
22+
23+
if (!common.hasCrypto)
24+
common.skip('missing crypto');
25+
26+
const assert = require('assert');
27+
const https = require('https');
28+
const fixtures = require('../common/fixtures');
29+
30+
const REQUEST_COUNT = 100;
31+
const agent = new https.Agent({ keepAlive: true, maxSockets: 1 });
32+
33+
const key = fixtures.readKey('agent1-key.pem');
34+
const cert = fixtures.readKey('agent1-cert.pem');
35+
const server = https.createServer({ key, cert }, common.mustCall((req, res) => {
36+
req.on('error', common.mustNotCall());
37+
res.writeHead(200);
38+
res.end();
39+
}, REQUEST_COUNT));
40+
41+
server.listen(0, common.mustCall(() => {
42+
const { port } = server.address();
43+
44+
async function run() {
45+
try {
46+
for (let i = 0; i < REQUEST_COUNT; i++) {
47+
await sendRequest(port);
48+
}
49+
} finally {
50+
agent.destroy();
51+
server.close();
52+
}
53+
}
54+
55+
run().then(common.mustCall());
56+
}));
57+
58+
function sendRequest(port) {
59+
let timeout;
60+
const promise = new Promise((resolve, reject) => {
61+
function done(err) {
62+
clearTimeout(timeout);
63+
if (err)
64+
reject(err);
65+
else
66+
resolve();
67+
}
68+
69+
const req = https.request({
70+
port,
71+
host: '127.0.0.1',
72+
rejectUnauthorized: false,
73+
method: 'POST',
74+
agent,
75+
headers: {
76+
'Content-Length': '0',
77+
'Expect': '100-continue',
78+
},
79+
}, common.mustCall((res) => {
80+
assert.strictEqual(res.statusCode, 200);
81+
res.resume();
82+
res.once('end', done);
83+
res.once('error', done);
84+
}));
85+
86+
timeout = setTimeout(() => {
87+
const err = new Error('request timed out');
88+
req.destroy(err);
89+
done(err);
90+
}, common.platformTimeout(5000));
91+
92+
req.once('error', done);
93+
94+
setTimeout(() => req.end(Buffer.alloc(0)), 0);
95+
});
96+
return promise.finally(() => clearTimeout(timeout));
97+
}

0 commit comments

Comments
0 (0)
You are visitor 01402520 — 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.