Skip to content

Commit 8345a82

Browse files
basti1302MylesBorins
authored andcommitted
async_hooks: add missing async_hooks destroys in AsyncReset
This adds missing async_hooks destroy calls for sockets (in _http_agent.js) and HTTP parsers. We need to emit a destroy in AsyncWrap#AsyncReset before assigning a new async_id when the instance has already been in use and is being recycled, because in that case, we have already emitted an init for the "old" async_id. This also removes a duplicated init call for HTTP parser: Each time a new parser was created, AsyncReset was being called via the C++ Parser class constructor (super constructor AsyncWrap) and also via Parser::Reinitialize. Backport-PR-URL: #23404 PR-URL: #23272 Fixes: #19859 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 523db5f commit 8345a82

16 files changed

+207
-40
lines changed

benchmark/http/bench-parser.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ function processHeader(header, n) {
3131
bench.start();
3232
for (var i = 0; i < n; i++) {
3333
parser.execute(header, 0, header.length);
34-
parser.reinitialize(REQUEST);
34+
parser.reinitialize(REQUEST, i > 0);
3535
}
3636
bench.end(n);
3737
}

benchmark/misc/freelist.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ const bench = common.createBenchmark(main, {
99
});
1010

1111
function main({ n }) {
12-
const FreeList = require('internal/freelist');
12+
const { FreeList } = require('internal/freelist');
1313
const poolSize = 1000;
1414
const list = new FreeList('test', poolSize, Object);
1515
var j;

lib/_http_agent.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
167167
var socket = this.freeSockets[name].shift();
168168
// Guard against an uninitialized or user supplied Socket.
169169
if (socket._handle && typeof socket._handle.asyncReset === 'function') {
170-
// Assign the handle a new asyncId and run any init() hooks.
170+
// Assign the handle a new asyncId and run any destroy()/init() hooks.
171171
socket._handle.asyncReset();
172172
socket[async_id_symbol] = socket._handle.getAsyncId();
173173
}

lib/_http_client.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ const {
4848
ERR_UNESCAPED_CHARACTERS
4949
} = require('internal/errors').codes;
5050
const { validateTimerDuration } = require('internal/timers');
51+
const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
5152

5253
const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/;
5354

@@ -625,7 +626,7 @@ function tickOnSocket(req, socket) {
625626
var parser = parsers.alloc();
626627
req.socket = socket;
627628
req.connection = socket;
628-
parser.reinitialize(HTTPParser.RESPONSE);
629+
parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);
629630
parser.socket = socket;
630631
parser.outgoing = req;
631632
req.parser = parser;

lib/_http_common.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
const { methods, HTTPParser } = internalBinding('http_parser');
2525

26-
const FreeList = require('internal/freelist');
26+
const { FreeList } = require('internal/freelist');
2727
const { ondrain } = require('internal/http');
2828
const incoming = require('_http_incoming');
2929
const {

lib/_http_server.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ const {
4242
defaultTriggerAsyncIdScope,
4343
getOrSetAsyncId
4444
} = require('internal/async_hooks');
45+
const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
4546
const { IncomingMessage } = require('_http_incoming');
4647
const {
4748
ERR_HTTP_HEADERS_SENT,
@@ -339,7 +340,7 @@ function connectionListenerInternal(server, socket) {
339340
socket.on('timeout', socketOnTimeout);
340341

341342
var parser = parsers.alloc();
342-
parser.reinitialize(HTTPParser.REQUEST);
343+
parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol]);
343344
parser.socket = socket;
344345
socket.parser = parser;
345346

lib/internal/freelist.js

+17-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
'use strict';
22

3+
const is_reused_symbol = Symbol('isReused');
4+
35
class FreeList {
46
constructor(name, max, ctor) {
57
this.name = name;
@@ -9,9 +11,15 @@ class FreeList {
911
}
1012

1113
alloc() {
12-
return this.list.length ?
13-
this.list.pop() :
14-
this.ctor.apply(this, arguments);
14+
let item;
15+
if (this.list.length > 0) {
16+
item = this.list.pop();
17+
item[is_reused_symbol] = true;
18+
} else {
19+
item = this.ctor.apply(this, arguments);
20+
item[is_reused_symbol] = false;
21+
}
22+
return item;
1523
}
1624

1725
free(obj) {
@@ -23,4 +31,9 @@ class FreeList {
2331
}
2432
}
2533

26-
module.exports = FreeList;
34+
module.exports = {
35+
FreeList,
36+
symbols: {
37+
is_reused_symbol
38+
}
39+
};

src/async_wrap.cc

+9
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,7 @@ AsyncWrap::AsyncWrap(Environment* env,
562562
CHECK_NE(provider, PROVIDER_NONE);
563563
CHECK_GE(object->InternalFieldCount(), 1);
564564

565+
async_id_ = -1;
565566
// Use AsyncReset() call to execute the init() callbacks.
566567
AsyncReset(execution_async_id, silent);
567568
}
@@ -605,6 +606,14 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
605606
// and reused over their lifetime. This way a new uid can be assigned when
606607
// the resource is pulled out of the pool and put back into use.
607608
void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
609+
if (async_id_ != -1) {
610+
// This instance was in use before, we have already emitted an init with
611+
// its previous async_id and need to emit a matching destroy for that
612+
// before generating a new async_id.
613+
EmitDestroy(env(), async_id_);
614+
}
615+
616+
// Now we can assign a new async_id_ to this instance.
608617
async_id_ =
609618
execution_async_id == -1 ? env()->new_async_id() : execution_async_id;
610619
trigger_async_id_ = env()->get_default_trigger_async_id();

src/node_http_parser.cc

+8-2
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,8 @@ class Parser : public AsyncWrap, public StreamListener {
465465
Environment* env = Environment::GetCurrent(args);
466466

467467
CHECK(args[0]->IsInt32());
468+
CHECK(args[1]->IsBoolean());
469+
bool isReused = args[1]->IsTrue();
468470
http_parser_type type =
469471
static_cast<http_parser_type>(args[0].As<Int32>()->Value());
470472

@@ -473,8 +475,12 @@ class Parser : public AsyncWrap, public StreamListener {
473475
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
474476
// Should always be called from the same context.
475477
CHECK_EQ(env, parser->env());
476-
// The parser is being reused. Reset the async id and call init() callbacks.
477-
parser->AsyncReset();
478+
// This parser has either just been created or it is being reused.
479+
// We must only call AsyncReset for the latter case, because AsyncReset has
480+
// already been called via the constructor for the former case.
481+
if (isReused) {
482+
parser->AsyncReset();
483+
}
478484
parser->Init(type);
479485
}
480486

test/async-hooks/test-graph.http.js

+2-11
Original file line numberDiff line numberDiff line change
@@ -38,24 +38,15 @@ process.on('exit', function() {
3838
{ type: 'HTTPPARSER',
3939
id: 'httpparser:1',
4040
triggerAsyncId: 'tcpserver:1' },
41-
{ type: 'HTTPPARSER',
42-
id: 'httpparser:2',
43-
triggerAsyncId: 'tcpserver:1' },
4441
{ type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
4542
{ type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' },
4643
{ type: 'TIMERWRAP', id: 'timer:1', triggerAsyncId: 'tcp:2' },
4744
{ type: 'HTTPPARSER',
48-
id: 'httpparser:3',
49-
triggerAsyncId: 'tcp:2' },
50-
{ type: 'HTTPPARSER',
51-
id: 'httpparser:4',
45+
id: 'httpparser:2',
5246
triggerAsyncId: 'tcp:2' },
5347
{ type: 'Timeout',
5448
id: 'timeout:2',
55-
triggerAsyncId: 'httpparser:4' },
56-
{ type: 'TIMERWRAP',
57-
id: 'timer:2',
58-
triggerAsyncId: 'httpparser:4' },
49+
triggerAsyncId: 'httpparser:2' },
5950
{ type: 'SHUTDOWNWRAP',
6051
id: 'shutdown:1',
6152
triggerAsyncId: 'tcp:2' } ]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
'use strict';
2+
// Flags: --expose-internals
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const { async_id_symbol } = require('internal/async_hooks').symbols;
6+
const async_hooks = require('async_hooks');
7+
const http = require('http');
8+
9+
// Regression test for https://github.com/nodejs/node/issues/19859
10+
// Checks that an http.Agent emits a destroy for the old asyncId before calling
11+
// asyncReset()s when reusing a socket handle. The setup is nearly identical to
12+
// parallel/test-async-hooks-http-agent (which focuses on the assertion that
13+
// a fresh asyncId is assigned to the net.Socket instance).
14+
15+
const destroyedIds = new Set();
16+
async_hooks.createHook({
17+
destroy: common.mustCallAtLeast((asyncId) => {
18+
destroyedIds.add(asyncId);
19+
}, 1)
20+
}).enable();
21+
22+
// Make sure a single socket is transparently reused for 2 requests.
23+
const agent = new http.Agent({
24+
keepAlive: true,
25+
keepAliveMsecs: Infinity,
26+
maxSockets: 1
27+
});
28+
29+
const server = http.createServer(common.mustCall((req, res) => {
30+
req.once('data', common.mustCallAtLeast(() => {
31+
res.writeHead(200, { 'Content-Type': 'text/plain' });
32+
res.write('foo');
33+
}));
34+
req.on('end', common.mustCall(() => {
35+
res.end('bar');
36+
}));
37+
}, 2)).listen(0, common.mustCall(() => {
38+
const port = server.address().port;
39+
const payload = 'hello world';
40+
41+
// First request. This is useless except for adding a socket to the
42+
// agent’s pool for reuse.
43+
const r1 = http.request({
44+
agent, port, method: 'POST'
45+
}, common.mustCall((res) => {
46+
// Remember which socket we used.
47+
const socket = res.socket;
48+
const asyncIdAtFirstRequest = socket[async_id_symbol];
49+
assert.ok(asyncIdAtFirstRequest > 0, `${asyncIdAtFirstRequest} > 0`);
50+
// Check that request and response share their socket.
51+
assert.strictEqual(r1.socket, socket);
52+
53+
res.on('data', common.mustCallAtLeast(() => {}));
54+
res.on('end', common.mustCall(() => {
55+
// setImmediate() to give the agent time to register the freed socket.
56+
setImmediate(common.mustCall(() => {
57+
// The socket is free for reuse now.
58+
assert.strictEqual(socket[async_id_symbol], -1);
59+
60+
// second request:
61+
const r2 = http.request({
62+
agent, port, method: 'POST'
63+
}, common.mustCall((res) => {
64+
assert.ok(destroyedIds.has(asyncIdAtFirstRequest));
65+
66+
// Empty payload, to hit the “right” code path.
67+
r2.end('');
68+
69+
res.on('data', common.mustCallAtLeast(() => {}));
70+
res.on('end', common.mustCall(() => {
71+
// Clean up to let the event loop stop.
72+
server.close();
73+
agent.destroy();
74+
}));
75+
}));
76+
77+
// Schedule a payload to be written immediately, but do not end the
78+
// request just yet.
79+
r2.write(payload);
80+
}));
81+
}));
82+
}));
83+
r1.end(payload);
84+
}));
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
'use strict';
2+
const common = require('../common');
3+
const Countdown = require('../common/countdown');
4+
const assert = require('assert');
5+
const async_hooks = require('async_hooks');
6+
const http = require('http');
7+
8+
// Regression test for https://github.com/nodejs/node/issues/19859.
9+
// Checks that matching destroys are emitted when creating new/reusing old http
10+
// parser instances.
11+
12+
const N = 50;
13+
const KEEP_ALIVE = 100;
14+
15+
const createdIds = [];
16+
const destroyedIds = [];
17+
async_hooks.createHook({
18+
init: common.mustCallAtLeast((asyncId, type) => {
19+
if (type === 'HTTPPARSER') {
20+
createdIds.push(asyncId);
21+
}
22+
}, N),
23+
destroy: (asyncId) => {
24+
destroyedIds.push(asyncId);
25+
}
26+
}).enable();
27+
28+
const server = http.createServer(function(req, res) {
29+
res.end('Hello');
30+
});
31+
32+
const keepAliveAgent = new http.Agent({
33+
keepAlive: true,
34+
keepAliveMsecs: KEEP_ALIVE,
35+
});
36+
37+
const countdown = new Countdown(N, () => {
38+
server.close(() => {
39+
// give the server sockets time to close (which will also free their
40+
// associated parser objects) after the server has been closed.
41+
setTimeout(() => {
42+
createdIds.forEach((createdAsyncId) => {
43+
assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0);
44+
});
45+
}, KEEP_ALIVE * 2);
46+
});
47+
});
48+
49+
server.listen(0, function() {
50+
for (let i = 0; i < N; ++i) {
51+
(function makeRequest() {
52+
http.get({
53+
port: server.address().port,
54+
agent: keepAliveAgent
55+
}, function(res) {
56+
countdown.dec();
57+
res.resume();
58+
});
59+
})();
60+
}
61+
});

test/parallel/test-freelist.js

+12-13
Original file line numberDiff line numberDiff line change
@@ -4,28 +4,27 @@
44

55
require('../common');
66
const assert = require('assert');
7-
const FreeList = require('internal/freelist');
7+
const { FreeList } = require('internal/freelist');
88

99
assert.strictEqual(typeof FreeList, 'function');
1010

11-
const flist1 = new FreeList('flist1', 3, String);
11+
const flist1 = new FreeList('flist1', 3, Object);
1212

1313
// Allocating when empty, should not change the list size
14-
const result = flist1.alloc('test');
15-
assert.strictEqual(typeof result, 'string');
16-
assert.strictEqual(result, 'test');
14+
const result = flist1.alloc();
15+
assert.strictEqual(typeof result, 'object');
1716
assert.strictEqual(flist1.list.length, 0);
1817

1918
// Exhaust the free list
20-
assert(flist1.free('test1'));
21-
assert(flist1.free('test2'));
22-
assert(flist1.free('test3'));
19+
assert(flist1.free({ id: 'test1' }));
20+
assert(flist1.free({ id: 'test2' }));
21+
assert(flist1.free({ id: 'test3' }));
2322

2423
// Now it should not return 'true', as max length is exceeded
25-
assert.strictEqual(flist1.free('test4'), false);
26-
assert.strictEqual(flist1.free('test5'), false);
24+
assert.strictEqual(flist1.free({ id: 'test4' }), false);
25+
assert.strictEqual(flist1.free({ id: 'test5' }), false);
2726

2827
// At this point 'alloc' should just return the stored values
29-
assert.strictEqual(flist1.alloc(), 'test3');
30-
assert.strictEqual(flist1.alloc(), 'test2');
31-
assert.strictEqual(flist1.alloc(), 'test1');
28+
assert.strictEqual(flist1.alloc().id, 'test3');
29+
assert.strictEqual(flist1.alloc().id, 'test2');
30+
assert.strictEqual(flist1.alloc().id, 'test1');

test/parallel/test-http-parser.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ function expectBody(expected) {
9595
throw new Error('hello world');
9696
};
9797

98-
parser.reinitialize(HTTPParser.REQUEST);
98+
parser.reinitialize(HTTPParser.REQUEST, true);
9999

100100
assert.throws(
101101
() => { parser.execute(request, 0, request.length); },
@@ -555,7 +555,7 @@ function expectBody(expected) {
555555
parser[kOnBody] = expectBody('ping');
556556
parser.execute(req1, 0, req1.length);
557557

558-
parser.reinitialize(REQUEST);
558+
parser.reinitialize(REQUEST, true);
559559
parser[kOnBody] = expectBody('pong');
560560
parser[kOnHeadersComplete] = onHeadersComplete2;
561561
parser.execute(req2, 0, req2.length);

test/parallel/test-internal-modules-expose.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,5 @@ const config = process.binding('config');
77

88
console.log(config, process.argv);
99

10-
assert.strictEqual(typeof require('internal/freelist'), 'function');
10+
assert.strictEqual(typeof require('internal/freelist').FreeList, 'function');
1111
assert.strictEqual(config.exposeInternals, true);

0 commit comments

Comments
 (0)