Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8345a82

Browse filesBrowse files
basti1302MylesBorins
basti1302
authored and
MylesBorins
committed
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 <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 523db5f commit 8345a82
Copy full SHA for 8345a82
Expand file treeCollapse file tree

16 files changed

+207
-40
lines changed
Open diff view settings
Collapse file

‎benchmark/http/bench-parser.js‎

Copy file name to clipboardExpand all lines: benchmark/http/bench-parser.js
+1-1Lines changed: 1 addition & 1 deletion
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
}
Collapse file

‎benchmark/misc/freelist.js‎

Copy file name to clipboardExpand all lines: benchmark/misc/freelist.js
+1-1Lines changed: 1 addition & 1 deletion
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;
Collapse file

‎lib/_http_agent.js‎

Copy file name to clipboardExpand all lines: lib/_http_agent.js
+1-1Lines changed: 1 addition & 1 deletion
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
}
Collapse file

‎lib/_http_client.js‎

Copy file name to clipboardExpand all lines: lib/_http_client.js
+2-1Lines changed: 2 additions & 1 deletion
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;
Collapse file

‎lib/_http_common.js‎

Copy file name to clipboardExpand all lines: lib/_http_common.js
+1-1Lines changed: 1 addition & 1 deletion
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 {
Collapse file

‎lib/_http_server.js‎

Copy file name to clipboardExpand all lines: lib/_http_server.js
+2-1Lines changed: 2 additions & 1 deletion
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

Collapse file

‎lib/internal/freelist.js‎

Copy file name to clipboardExpand all lines: lib/internal/freelist.js
+17-4Lines changed: 17 additions & 4 deletions
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+
};
Collapse file

‎src/async_wrap.cc‎

Copy file name to clipboardExpand all lines: src/async_wrap.cc
+9Lines changed: 9 additions & 0 deletions
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();
Collapse file

‎src/node_http_parser.cc‎

Copy file name to clipboardExpand all lines: src/node_http_parser.cc
+8-2Lines changed: 8 additions & 2 deletions
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

Collapse file

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

Copy file name to clipboardExpand all lines: test/async-hooks/test-graph.http.js
+2-11Lines changed: 2 additions & 11 deletions
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' } ]

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.