Skip to content

Commit 26d145a

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: #23410 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 0d241ba commit 26d145a

16 files changed

+207
-40
lines changed

benchmark/http/bench-parser.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ function processHeader(header, n) {
3535
bench.start();
3636
for (var i = 0; i < n; i++) {
3737
parser.execute(header, 0, header.length);
38-
parser.reinitialize(REQUEST);
38+
parser.reinitialize(REQUEST, i > 0);
3939
}
4040
bench.end(n);
4141
}

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(conf) {
12-
const FreeList = require('internal/freelist');
12+
const { FreeList } = require('internal/freelist');
1313
const n = conf.n;
1414
const poolSize = 1000;
1515
const list = new FreeList('test', poolSize, Object);

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
@@ -39,6 +39,7 @@ const { Buffer } = require('buffer');
3939
const { urlToOptions, searchParamsSymbol } = require('internal/url');
4040
const { outHeadersKey, ondrain } = require('internal/http');
4141
const { nextTick } = require('internal/process/next_tick');
42+
const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
4243

4344
// The actual list of disallowed characters in regexp form is more like:
4445
// /[^A-Za-z0-9\-._~!$&'()*+,;=/:@]/
@@ -618,7 +619,7 @@ function tickOnSocket(req, socket) {
618619
var parser = parsers.alloc();
619620
req.socket = socket;
620621
req.connection = socket;
621-
parser.reinitialize(HTTPParser.RESPONSE);
622+
parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);
622623
parser.socket = socket;
623624
parser.incoming = null;
624625
parser.outgoing = req;

lib/_http_common.js

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

2424
const { methods, HTTPParser } = process.binding('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

4748
const kServerResponse = Symbol('ServerResponse');
@@ -331,7 +332,7 @@ function connectionListenerInternal(server, socket) {
331332
socket.on('timeout', socketOnTimeout);
332333

333334
var parser = parsers.alloc();
334-
parser.reinitialize(HTTPParser.REQUEST);
335+
parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol]);
335336
parser.socket = socket;
336337
socket.parser = parser;
337338
parser.incoming = null;

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
@@ -641,6 +641,7 @@ AsyncWrap::AsyncWrap(Environment* env,
641641
// Shift provider value over to prevent id collision.
642642
persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider_type_);
643643

644+
async_id_ = -1;
644645
// Use AsyncReset() call to execute the init() callbacks.
645646
AsyncReset(execution_async_id, silent);
646647
}
@@ -681,6 +682,14 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
681682
// and reused over their lifetime. This way a new uid can be assigned when
682683
// the resource is pulled out of the pool and put back into use.
683684
void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
685+
if (async_id_ != -1) {
686+
// This instance was in use before, we have already emitted an init with
687+
// its previous async_id and need to emit a matching destroy for that
688+
// before generating a new async_id.
689+
EmitDestroy(env(), async_id_);
690+
}
691+
692+
// Now we can assign a new async_id_ to this instance.
684693
async_id_ =
685694
execution_async_id == -1 ? env()->new_async_id() : execution_async_id;
686695
trigger_async_id_ = env()->get_default_trigger_async_id();

src/node_http_parser.cc

+9-2
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,9 @@ class Parser : public AsyncWrap {
464464
static void Reinitialize(const FunctionCallbackInfo<Value>& args) {
465465
Environment* env = Environment::GetCurrent(args);
466466

467+
CHECK(args[0]->IsInt32());
468+
CHECK(args[1]->IsBoolean());
469+
bool isReused = args[1]->IsTrue();
467470
http_parser_type type =
468471
static_cast<http_parser_type>(args[0]->Int32Value());
469472

@@ -472,8 +475,12 @@ class Parser : public AsyncWrap {
472475
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
473476
// Should always be called from the same context.
474477
CHECK_EQ(env, parser->env());
475-
// The parser is being reused. Reset the async id and call init() callbacks.
476-
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+
}
477484
parser->Init(type);
478485
}
479486

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,83 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const async_id_symbol = process.binding('async_wrap').async_id_symbol;
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 an http.Agent emits a destroy for the old asyncId before calling
10+
// asyncReset()s when reusing a socket handle. The setup is nearly identical to
11+
// parallel/test-async-hooks-http-agent (which focuses on the assertion that
12+
// a fresh asyncId is assigned to the net.Socket instance).
13+
14+
const destroyedIds = new Set();
15+
async_hooks.createHook({
16+
destroy: common.mustCallAtLeast((asyncId) => {
17+
destroyedIds.add(asyncId);
18+
}, 1)
19+
}).enable();
20+
21+
// Make sure a single socket is transparently reused for 2 requests.
22+
const agent = new http.Agent({
23+
keepAlive: true,
24+
keepAliveMsecs: Infinity,
25+
maxSockets: 1
26+
});
27+
28+
const server = http.createServer(common.mustCall((req, res) => {
29+
req.once('data', common.mustCallAtLeast(() => {
30+
res.writeHead(200, { 'Content-Type': 'text/plain' });
31+
res.write('foo');
32+
}));
33+
req.on('end', common.mustCall(() => {
34+
res.end('bar');
35+
}));
36+
}, 2)).listen(0, common.mustCall(() => {
37+
const port = server.address().port;
38+
const payload = 'hello world';
39+
40+
// First request. This is useless except for adding a socket to the
41+
// agent’s pool for reuse.
42+
const r1 = http.request({
43+
agent, port, method: 'POST'
44+
}, common.mustCall((res) => {
45+
// Remember which socket we used.
46+
const socket = res.socket;
47+
const asyncIdAtFirstRequest = socket[async_id_symbol];
48+
assert.ok(asyncIdAtFirstRequest > 0, `${asyncIdAtFirstRequest} > 0`);
49+
// Check that request and response share their socket.
50+
assert.strictEqual(r1.socket, socket);
51+
52+
res.on('data', common.mustCallAtLeast(() => {}));
53+
res.on('end', common.mustCall(() => {
54+
// setImmediate() to give the agent time to register the freed socket.
55+
setImmediate(common.mustCall(() => {
56+
// The socket is free for reuse now.
57+
assert.strictEqual(socket[async_id_symbol], -1);
58+
59+
// second request:
60+
const r2 = http.request({
61+
agent, port, method: 'POST'
62+
}, common.mustCall((res) => {
63+
assert.ok(destroyedIds.has(asyncIdAtFirstRequest));
64+
65+
// Empty payload, to hit the “right” code path.
66+
r2.end('');
67+
68+
res.on('data', common.mustCallAtLeast(() => {}));
69+
res.on('end', common.mustCall(() => {
70+
// Clean up to let the event loop stop.
71+
server.close();
72+
agent.destroy();
73+
}));
74+
}));
75+
76+
// Schedule a payload to be written immediately, but do not end the
77+
// request just yet.
78+
r2.write(payload);
79+
}));
80+
}));
81+
}));
82+
r1.end(payload);
83+
}));
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(function() {
101101
parser.execute(request, 0, request.length);
@@ -554,7 +554,7 @@ function expectBody(expected) {
554554
parser[kOnBody] = expectBody('ping');
555555
parser.execute(req1, 0, req1.length);
556556

557-
parser.reinitialize(REQUEST);
557+
parser.reinitialize(REQUEST, true);
558558
parser[kOnBody] = expectBody('pong');
559559
parser[kOnHeadersComplete] = onHeadersComplete2;
560560
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)