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 26d145a

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: #23410 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 0d241ba commit 26d145a
Copy full SHA for 26d145a
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
@@ -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
}
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(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);
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
@@ -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;
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 } = 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 {
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

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;
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
@@ -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();
Collapse file

‎src/node_http_parser.cc‎

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

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.