diff options
author | Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> | 2019-05-30 23:54:41 +0300 |
---|---|---|
committer | Beth Griggs <Bethany.Griggs@uk.ibm.com> | 2019-07-16 19:37:59 +0300 |
commit | 65ef26fdcbc59ee861010938f7b8057619d79b4a (patch) | |
tree | f3a17d7756a7ab9715833fdd17d41e6e0cb15759 /src | |
parent | 08a32fbf57a1d8735a1e50888ce3cde8f4089f23 (diff) |
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 <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Diffstat (limited to 'src')
-rw-r--r-- | src/async_wrap.cc | 31 | ||||
-rw-r--r-- | src/async_wrap.h | 11 | ||||
-rw-r--r-- | src/node_http_parser.cc | 3 |
3 files changed, 27 insertions, 18 deletions
diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 86142d13a32..6eb06cc81e9 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> 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<Value>& args) { void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& 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<Value>& args) { AsyncWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); double execution_async_id = - args[0]->IsNumber() ? args[0].As<Number>()->Value() : -1; + args[0]->IsNumber() ? args[0].As<Number>()->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<Value>& args) { CHECK(args[0]->IsNumber()); @@ -474,7 +479,7 @@ void AsyncWrap::Initialize(Local<Object> 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 c342898b143..3e22fa20b68 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -108,10 +108,12 @@ class AsyncWrap : public BaseObject { AsyncWrap(Environment* env, v8::Local<v8::Object> object, ProviderType provider, - double execution_async_id = -1); + double execution_async_id = kInvalidAsyncId); virtual ~AsyncWrap(); + static constexpr double kInvalidAsyncId = -1; + static v8::Local<v8::FunctionTemplate> 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<v8::Value> MakeCallback(const v8::Local<v8::Function> 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 b29cdf4b28b..c2cd7a213bb 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<Value>& 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(); } |