From 8a3ffca8791e7c1cf9f6418b59e54e86f8a94c45 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Thu, 30 May 2019 22:54:41 +0200 Subject: [PATCH] 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 https://github.com/nodejs/node/pull/27477, a full backport would require also https://github.com/nodejs/node/pull/25094 which has a dont-land-on-v10.x label on it. Fixes: https://github.com/nodejs/node/issues/26961 Backport-PR-URL: https://github.com/nodejs/node/pull/27986 PR-URL: https://github.com/nodejs/node/pull/27477 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina Reviewed-By: Rich Trott --- src/async_wrap.cc | 31 ++++++----- src/async_wrap.h | 11 ++-- src/node_http_parser.cc | 3 +- ...sync-hooks-http-parser-nodouble-destroy.js | 53 +++++++++++++++++++ 4 files changed, 80 insertions(+), 18 deletions(-) create mode 100644 test/parallel/test-async-hooks-http-parser-nodouble-destroy.js diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 86142d13a32a8e..6eb06cc81e981a 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -176,7 +176,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) { class PromiseWrap : public AsyncWrap { public: PromiseWrap(Environment* env, Local object, bool silent) - : AsyncWrap(env, object, PROVIDER_PROMISE, -1, silent) { + : AsyncWrap(env, object, PROVIDER_PROMISE, kInvalidAsyncId, silent) { MakeWeak(); } @@ -382,7 +382,7 @@ static void RegisterDestroyHook(const FunctionCallbackInfo& args) { void AsyncWrap::GetAsyncId(const FunctionCallbackInfo& args) { AsyncWrap* wrap; - args.GetReturnValue().Set(-1); + args.GetReturnValue().Set(kInvalidAsyncId); ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); args.GetReturnValue().Set(wrap->get_async_id()); } @@ -409,10 +409,15 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo& args) { AsyncWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); double execution_async_id = - args[0]->IsNumber() ? args[0].As()->Value() : -1; + args[0]->IsNumber() ? args[0].As()->Value() : kInvalidAsyncId; wrap->AsyncReset(execution_async_id); } +void AsyncWrap::EmitDestroy() { + AsyncWrap::EmitDestroy(env(), async_id_); + // Ensure no double destroy is emitted via AsyncReset(). + async_id_ = kInvalidAsyncId; +} void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo& args) { CHECK(args[0]->IsNumber()); @@ -474,7 +479,7 @@ void AsyncWrap::Initialize(Local target, // kDefaultTriggerAsyncId: Write the id of the resource responsible for a // handle's creation just before calling the new handle's constructor. // After the new handle is constructed kDefaultTriggerAsyncId is set back - // to -1. + // to kInvalidAsyncId. FORCE_SET_TARGET_FIELD(target, "async_id_fields", env->async_hooks()->async_id_fields().GetJSArray()); @@ -558,7 +563,7 @@ AsyncWrap::AsyncWrap(Environment* env, CHECK_NE(provider, PROVIDER_NONE); CHECK_GE(object->InternalFieldCount(), 1); - async_id_ = -1; + async_id_ = kInvalidAsyncId; // Use AsyncReset() call to execute the init() callbacks. AsyncReset(execution_async_id, silent); } @@ -566,7 +571,7 @@ AsyncWrap::AsyncWrap(Environment* env, AsyncWrap::~AsyncWrap() { EmitTraceEventDestroy(); - EmitDestroy(env(), get_async_id()); + EmitDestroy(); } void AsyncWrap::EmitTraceEventDestroy() { @@ -602,16 +607,16 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) { // and reused over their lifetime. This way a new uid can be assigned when // the resource is pulled out of the pool and put back into use. void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { - if (async_id_ != -1) { + if (async_id_ != kInvalidAsyncId) { // This instance was in use before, we have already emitted an init with // its previous async_id and need to emit a matching destroy for that // before generating a new async_id. - EmitDestroy(env(), async_id_); + EmitDestroy(); } // Now we can assign a new async_id_ to this instance. - async_id_ = - execution_async_id == -1 ? env()->new_async_id() : execution_async_id; + async_id_ = execution_async_id == kInvalidAsyncId ? env()->new_async_id() + : execution_async_id; trigger_async_id_ = env()->get_default_trigger_async_id(); switch (provider_type()) { @@ -693,7 +698,7 @@ async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) { // Environment::GetCurrent() allocates a Local<> handle. HandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); - if (env == nullptr) return -1; + if (env == nullptr) return AsyncWrap::kInvalidAsyncId; return env->execution_async_id(); } @@ -702,7 +707,7 @@ async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) { // Environment::GetCurrent() allocates a Local<> handle. HandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); - if (env == nullptr) return -1; + if (env == nullptr) return AsyncWrap::kInvalidAsyncId; return env->trigger_async_id(); } @@ -727,7 +732,7 @@ async_context EmitAsyncInit(Isolate* isolate, CHECK_NOT_NULL(env); // Initialize async context struct - if (trigger_async_id == -1) + if (trigger_async_id == AsyncWrap::kInvalidAsyncId) trigger_async_id = env->get_default_trigger_async_id(); async_context context = { diff --git a/src/async_wrap.h b/src/async_wrap.h index c342898b1431ce..3e22fa20b681e1 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -108,10 +108,12 @@ class AsyncWrap : public BaseObject { AsyncWrap(Environment* env, v8::Local object, ProviderType provider, - double execution_async_id = -1); + double execution_async_id = kInvalidAsyncId); virtual ~AsyncWrap(); + static constexpr double kInvalidAsyncId = -1; + static v8::Local GetConstructorTemplate( Environment* env); @@ -137,6 +139,8 @@ class AsyncWrap : public BaseObject { static void EmitAfter(Environment* env, double async_id); static void EmitPromiseResolve(Environment* env, double async_id); + void EmitDestroy(); + void EmitTraceEventBefore(); static void EmitTraceEventAfter(ProviderType type, double async_id); void EmitTraceEventDestroy(); @@ -149,7 +153,8 @@ class AsyncWrap : public BaseObject { inline double get_trigger_async_id() const; - void AsyncReset(double execution_async_id = -1, bool silent = false); + void AsyncReset(double execution_async_id = kInvalidAsyncId, + bool silent = false); // Only call these within a valid HandleScope. v8::MaybeLocal MakeCallback(const v8::Local cb, @@ -202,7 +207,7 @@ class AsyncWrap : public BaseObject { inline AsyncWrap(); const ProviderType provider_type_; // Because the values may be Reset(), cannot be made const. - double async_id_ = -1; + double async_id_ = kInvalidAsyncId; double trigger_async_id_; }; diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index b29cdf4b28b9be..c2cd7a213bbdde 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -383,14 +383,13 @@ class Parser : public AsyncWrap, public StreamListener { static void Free(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); // Since the Parser destructor isn't going to run the destroy() callbacks // it needs to be triggered manually. parser->EmitTraceEventDestroy(); - parser->EmitDestroy(env, parser->get_async_id()); + parser->EmitDestroy(); } diff --git a/test/parallel/test-async-hooks-http-parser-nodouble-destroy.js b/test/parallel/test-async-hooks-http-parser-nodouble-destroy.js new file mode 100644 index 00000000000000..db512380be924a --- /dev/null +++ b/test/parallel/test-async-hooks-http-parser-nodouble-destroy.js @@ -0,0 +1,53 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { createHook } = require('async_hooks'); +const http = require('http'); + +// Regression test for https://github.com/nodejs/node/issues/19859. +// Checks that matching destroys are emitted when creating new/reusing old http +// parser instances. + +const httpParsers = []; +const dupDestroys = []; +const destroyed = []; + +createHook({ + init(asyncId, type, triggerAsyncId, resource) { + if (type === 'HTTPPARSER') { + httpParsers.push(asyncId); + } + }, + destroy(asyncId) { + if (destroyed.includes(asyncId)) { + dupDestroys.push(asyncId); + } else { + destroyed.push(asyncId); + } + } +}).enable(); + +const server = http.createServer((req, res) => { + res.end(); +}); + +server.listen(common.mustCall(() => { + http.get({ port: server.address().port }, common.mustCall(() => { + server.close(common.mustCall(() => { + server.listen(common.mustCall(() => { + http.get({ port: server.address().port }, common.mustCall(() => { + server.close(common.mustCall(() => { + setTimeout(common.mustCall(verify), 200); + })); + })); + })); + })); + })); +})); + +function verify() { + assert.strictEqual(httpParsers.length, 4); + + assert.strictEqual(dupDestroys.length, 0); + httpParsers.forEach((id) => assert.ok(destroyed.includes(id))); +}