FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

async_hooks: avoid double-destroy HTTPParser · nodejs/node@8a3ffca · GitHub

/ node Public

Commit 8a3ffca

Browse files
Flarna
authored and
BethGriggs
committed
async_hooks: avoid double-destroy HTTPParser
Avoid that destroy hook is invoked twice - once via `Parser::Free()` and once again via `Parser::Reinitialize()` by clearing the async_id in `EmitDestroy()`. Partial backport of #27477, a full backport would require also #25094 which has a dont-land-on-v10.x label on it. Fixes: #26961 Backport-PR-URL: #27986 PR-URL: #27477 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 8516da1 commit 8a3ffca

4 files changed

Lines changed: 80 additions & 18 deletions

File tree

‎src/async_wrap.cc‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
176176
class PromiseWrap : public AsyncWrap {
177177
public:
178178
PromiseWrap(Environment* env, Local<Object> object, bool silent)
179-
: AsyncWrap(env, object, PROVIDER_PROMISE, -1, silent) {
179+
: AsyncWrap(env, object, PROVIDER_PROMISE, kInvalidAsyncId, silent) {
180180
MakeWeak();
181181
}
182182

@@ -382,7 +382,7 @@ static void RegisterDestroyHook(const FunctionCallbackInfo<Value>& args) {
382382

383383
void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& args) {
384384
AsyncWrap* wrap;
385-
args.GetReturnValue().Set(-1);
385+
args.GetReturnValue().Set(kInvalidAsyncId);
386386
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
387387
args.GetReturnValue().Set(wrap->get_async_id());
388388
}
@@ -409,10 +409,15 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo<Value>& args) {
409409
AsyncWrap* wrap;
410410
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
411411
double execution_async_id =
412-
args[0]->IsNumber() ? args[0].As<Number>()->Value() : -1;
412+
args[0]->IsNumber() ? args[0].As<Number>()->Value() : kInvalidAsyncId;
413413
wrap->AsyncReset(execution_async_id);
414414
}
415415

416+
void AsyncWrap::EmitDestroy() {
417+
AsyncWrap::EmitDestroy(env(), async_id_);
418+
// Ensure no double destroy is emitted via AsyncReset().
419+
async_id_ = kInvalidAsyncId;
420+
}
416421

417422
void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo<Value>& args) {
418423
CHECK(args[0]->IsNumber());
@@ -474,7 +479,7 @@ void AsyncWrap::Initialize(Local<Object> target,
474479
// kDefaultTriggerAsyncId: Write the id of the resource responsible for a
475480
// handle's creation just before calling the new handle's constructor.
476481
// After the new handle is constructed kDefaultTriggerAsyncId is set back
477-
// to -1.
482+
// to kInvalidAsyncId.
478483
FORCE_SET_TARGET_FIELD(target,
479484
"async_id_fields",
480485
env->async_hooks()->async_id_fields().GetJSArray());
@@ -558,15 +563,15 @@ AsyncWrap::AsyncWrap(Environment* env,
558563
CHECK_NE(provider, PROVIDER_NONE);
559564
CHECK_GE(object->InternalFieldCount(), 1);
560565

561-
async_id_ = -1;
566+
async_id_ = kInvalidAsyncId;
562567
// Use AsyncReset() call to execute the init() callbacks.
563568
AsyncReset(execution_async_id, silent);
564569
}
565570

566571

567572
AsyncWrap::~AsyncWrap() {
568573
EmitTraceEventDestroy();
569-
EmitDestroy(env(), get_async_id());
574+
EmitDestroy();
570575
}
571576

572577
void AsyncWrap::EmitTraceEventDestroy() {
@@ -602,16 +607,16 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
602607
// and reused over their lifetime. This way a new uid can be assigned when
603608
// the resource is pulled out of the pool and put back into use.
604609
void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
605-
if (async_id_ != -1) {
610+
if (async_id_ != kInvalidAsyncId) {
606611
// This instance was in use before, we have already emitted an init with
607612
// its previous async_id and need to emit a matching destroy for that
608613
// before generating a new async_id.
609-
EmitDestroy(env(), async_id_);
614+
EmitDestroy();
610615
}
611616

612617
// Now we can assign a new async_id_ to this instance.
613-
async_id_ =
614-
execution_async_id == -1 ? env()->new_async_id() : execution_async_id;
618+
async_id_ = execution_async_id == kInvalidAsyncId ? env()->new_async_id()
619+
: execution_async_id;
615620
trigger_async_id_ = env()->get_default_trigger_async_id();
616621

617622
switch (provider_type()) {
@@ -693,7 +698,7 @@ async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
693698
// Environment::GetCurrent() allocates a Local<> handle.
694699
HandleScope handle_scope(isolate);
695700
Environment* env = Environment::GetCurrent(isolate);
696-
if (env == nullptr) return -1;
701+
if (env == nullptr) return AsyncWrap::kInvalidAsyncId;
697702
return env->execution_async_id();
698703
}
699704

@@ -702,7 +707,7 @@ async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
702707
// Environment::GetCurrent() allocates a Local<> handle.
703708
HandleScope handle_scope(isolate);
704709
Environment* env = Environment::GetCurrent(isolate);
705-
if (env == nullptr) return -1;
710+
if (env == nullptr) return AsyncWrap::kInvalidAsyncId;
706711
return env->trigger_async_id();
707712
}
708713

@@ -727,7 +732,7 @@ async_context EmitAsyncInit(Isolate* isolate,
727732
CHECK_NOT_NULL(env);
728733

729734
// Initialize async context struct
730-
if (trigger_async_id == -1)
735+
if (trigger_async_id == AsyncWrap::kInvalidAsyncId)
731736
trigger_async_id = env->get_default_trigger_async_id();
732737

733738
async_context context = {

‎src/async_wrap.h‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,10 +108,12 @@ class AsyncWrap : public BaseObject {
108108
AsyncWrap(Environment* env,
109109
v8::Local<v8::Object> object,
110110
ProviderType provider,
111-
double execution_async_id = -1);
111+
double execution_async_id = kInvalidAsyncId);
112112

113113
virtual ~AsyncWrap();
114114

115+
static constexpr double kInvalidAsyncId = -1;
116+
115117
static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
116118
Environment* env);
117119

@@ -137,6 +139,8 @@ class AsyncWrap : public BaseObject {
137139
static void EmitAfter(Environment* env, double async_id);
138140
static void EmitPromiseResolve(Environment* env, double async_id);
139141

142+
void EmitDestroy();
143+
140144
void EmitTraceEventBefore();
141145
static void EmitTraceEventAfter(ProviderType type, double async_id);
142146
void EmitTraceEventDestroy();
@@ -149,7 +153,8 @@ class AsyncWrap : public BaseObject {
149153

150154
inline double get_trigger_async_id() const;
151155

152-
void AsyncReset(double execution_async_id = -1, bool silent = false);
156+
void AsyncReset(double execution_async_id = kInvalidAsyncId,
157+
bool silent = false);
153158

154159
// Only call these within a valid HandleScope.
155160
v8::MaybeLocal<v8::Value> MakeCallback(const v8::Local<v8::Function> cb,
@@ -202,7 +207,7 @@ class AsyncWrap : public BaseObject {
202207
inline AsyncWrap();
203208
const ProviderType provider_type_;
204209
// Because the values may be Reset(), cannot be made const.
205-
double async_id_ = -1;
210+
double async_id_ = kInvalidAsyncId;
206211
double trigger_async_id_;
207212
};
208213

‎src/node_http_parser.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -383,14 +383,13 @@ class Parser : public AsyncWrap, public StreamListener {
383383

384384

385385
static void Free(const FunctionCallbackInfo<Value>& args) {
386-
Environment* env = Environment::GetCurrent(args);
387386
Parser* parser;
388387
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
389388

390389
// Since the Parser destructor isn't going to run the destroy() callbacks
391390
// it needs to be triggered manually.
392391
parser->EmitTraceEventDestroy();
393-
parser->EmitDestroy(env, parser->get_async_id());
392+
parser->EmitDestroy();
394393
}
395394

396395

‎test/parallel/test-async-hooks-http-parser-nodouble-destroy.js‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const { createHook } = require('async_hooks');
5+
const http = require('http');
6+
7+
// Regression test for https://github.com/nodejs/node/issues/19859.
8+
// Checks that matching destroys are emitted when creating new/reusing old http
9+
// parser instances.
10+
11+
const httpParsers = [];
12+
const dupDestroys = [];
13+
const destroyed = [];
14+
15+
createHook({
16+
init(asyncId, type, triggerAsyncId, resource) {
17+
if (type === 'HTTPPARSER') {
18+
httpParsers.push(asyncId);
19+
}
20+
},
21+
destroy(asyncId) {
22+
if (destroyed.includes(asyncId)) {
23+
dupDestroys.push(asyncId);
24+
} else {
25+
destroyed.push(asyncId);
26+
}
27+
}
28+
}).enable();
29+
30+
const server = http.createServer((req, res) => {
31+
res.end();
32+
});
33+
34+
server.listen(common.mustCall(() => {
35+
http.get({ port: server.address().port }, common.mustCall(() => {
36+
server.close(common.mustCall(() => {
37+
server.listen(common.mustCall(() => {
38+
http.get({ port: server.address().port }, common.mustCall(() => {
39+
server.close(common.mustCall(() => {
40+
setTimeout(common.mustCall(verify), 200);
41+
}));
42+
}));
43+
}));
44+
}));
45+
}));
46+
}));
47+
48+
function verify() {
49+
assert.strictEqual(httpParsers.length, 4);
50+
51+
assert.strictEqual(dupDestroys.length, 0);
52+
httpParsers.forEach((id) => assert.ok(destroyed.includes(id)));
53+
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL