From a4e9487aae4297fbf400b3d0d3933a35affa557c Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 20 Oct 2015 12:18:15 -0600 Subject: [PATCH 1/3] async_wrap: allow some hooks to be optional Only enforce that the init callback is passed to setupHooks(). The remaining hooks can be optionally passed. Throw if async_wrap.enable() runs before setting the init callback or if setupHooks() is called while async wrap is enabled. Add test to verify calls throw appropriately. PR-URL: https://github.com/nodejs/node/pull/3461 Reviewed-By: Fedor Indutny --- src/async-wrap.cc | 18 ++++++++++----- .../parallel/test-async-wrap-throw-no-init.js | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-async-wrap-throw-no-init.js diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 4f27e5116dca8d..767e76831c3d04 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -103,6 +103,9 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local wrapper) { static void EnableHooksJS(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + Local init_fn = env->async_hooks_init_function(); + if (init_fn.IsEmpty() || !init_fn->IsFunction()) + return env->ThrowTypeError("init callback is not assigned to a function"); env->async_hooks()->set_enable_callbacks(1); } @@ -116,13 +119,18 @@ static void DisableHooksJS(const FunctionCallbackInfo& args) { static void SetupHooks(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - CHECK(args[0]->IsFunction()); - CHECK(args[1]->IsFunction()); - CHECK(args[2]->IsFunction()); + if (env->async_hooks()->callbacks_enabled()) + return env->ThrowError("hooks should not be set while also enabled"); + + if (!args[0]->IsFunction()) + return env->ThrowTypeError("init callback must be a function"); env->set_async_hooks_init_function(args[0].As()); - env->set_async_hooks_pre_function(args[1].As()); - env->set_async_hooks_post_function(args[2].As()); + + if (args[1]->IsFunction()) + env->set_async_hooks_pre_function(args[1].As()); + if (args[2]->IsFunction()) + env->set_async_hooks_post_function(args[2].As()); } diff --git a/test/parallel/test-async-wrap-throw-no-init.js b/test/parallel/test-async-wrap-throw-no-init.js new file mode 100644 index 00000000000000..b2f60f3215e850 --- /dev/null +++ b/test/parallel/test-async-wrap-throw-no-init.js @@ -0,0 +1,22 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const async_wrap = process.binding('async_wrap'); + + +assert.throws(function() { + async_wrap.setupHooks(null); +}, /init callback must be a function/); + +assert.throws(function() { + async_wrap.enable(); +}, /init callback is not assigned to a function/); + +// Should not throw +async_wrap.setupHooks(() => {}); +async_wrap.enable(); + +assert.throws(function() { + async_wrap.setupHooks(() => {}); +}, /hooks should not be set while also enabled/); From 80a66ba6aebd7f749dff6c9a650da79d83bb732e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 19 Oct 2015 17:14:05 -0600 Subject: [PATCH 2/3] async_wrap: new instances get uid New instances of AsyncWrap are automatically assigned a unique id. The value will be used in future commits to communicate additional information via the async hooks. While the largest value we can reliably communicate to JS is 2^53, even if a new AsyncWrap is created every 100ns the uid won't reach its end for 28.5 years. PR-URL: https://github.com/nodejs/node/pull/3461 Reviewed-By: Fedor Indutny --- src/async-wrap-inl.h | 8 +++++++- src/async-wrap.h | 3 +++ src/env-inl.h | 5 +++++ src/env.h | 3 +++ 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index cac8175889dfbf..9aaf67d3a8c518 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -17,7 +17,8 @@ inline AsyncWrap::AsyncWrap(Environment* env, v8::Local object, ProviderType provider, AsyncWrap* parent) - : BaseObject(env, object), bits_(static_cast(provider) << 1) { + : BaseObject(env, object), bits_(static_cast(provider) << 1), + uid_(env->get_async_wrap_uid()) { CHECK_NE(provider, PROVIDER_NONE); CHECK_GE(object->InternalFieldCount(), 1); @@ -66,6 +67,11 @@ inline AsyncWrap::ProviderType AsyncWrap::provider_type() const { } +inline int64_t AsyncWrap::get_uid() const { + return uid_; +} + + inline v8::Local AsyncWrap::MakeCallback( const v8::Local symbol, int argc, diff --git a/src/async-wrap.h b/src/async-wrap.h index 330f3454f42d2c..5fbd2309383d87 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -55,6 +55,8 @@ class AsyncWrap : public BaseObject { inline ProviderType provider_type() const; + inline int64_t get_uid() const; + // Only call these within a valid HandleScope. v8::Local MakeCallback(const v8::Local cb, int argc, @@ -76,6 +78,7 @@ class AsyncWrap : public BaseObject { // expected the context object will receive a _asyncQueue object property // that will be used to call pre/post in MakeCallback. uint32_t bits_; + const int64_t uid_; }; void LoadAsyncWrapperInfo(Environment* env); diff --git a/src/env-inl.h b/src/env-inl.h index 2d965f9a519232..f73e9c6ba2000a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -210,6 +210,7 @@ inline Environment::Environment(v8::Local context, using_domains_(false), printed_error_(false), trace_sync_io_(false), + async_wrap_uid_(0), debugger_agent_(this), http_parser_buffer_(nullptr), context_(context->GetIsolate(), context) { @@ -359,6 +360,10 @@ inline void Environment::set_trace_sync_io(bool value) { trace_sync_io_ = value; } +inline int64_t Environment::get_async_wrap_uid() { + return ++async_wrap_uid_; +} + inline uint32_t* Environment::heap_statistics_buffer() const { CHECK_NE(heap_statistics_buffer_, nullptr); return heap_statistics_buffer_; diff --git a/src/env.h b/src/env.h index c558764c671a21..cdfd19c9740e31 100644 --- a/src/env.h +++ b/src/env.h @@ -446,6 +446,8 @@ class Environment { void PrintSyncTrace() const; inline void set_trace_sync_io(bool value); + inline int64_t get_async_wrap_uid(); + bool KickNextTick(); inline uint32_t* heap_statistics_buffer() const; @@ -541,6 +543,7 @@ class Environment { bool using_domains_; bool printed_error_; bool trace_sync_io_; + int64_t async_wrap_uid_; debugger::Agent debugger_agent_; HandleWrapQueue handle_wrap_queue_; From bb1bd7639554766a88b8f7aef8891bf8249e613e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 20 Oct 2015 12:20:10 -0600 Subject: [PATCH 3/3] async_wrap: call callback in destructor Call a user's callback to notify that the handle has been destroyed. Only pass the id of the AsyncWrap instance since the object no longer exists. The object that's being destructed should never be inspected within the callback or any time afterward. This commit make a breaking change. The init callback will now be passed arguments in the order of provider, id, parent. PR-URL: https://github.com/nodejs/node/pull/3461 Reviewed-By: Fedor Indutny --- src/async-wrap-inl.h | 19 ++++++++++++++++++- src/async-wrap.cc | 3 +++ src/async-wrap.h | 2 +- src/env.h | 1 + ...st-async-wrap-disabled-propagate-parent.js | 2 +- .../test-async-wrap-propagate-parent.js | 2 +- 6 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index 9aaf67d3a8c518..1d9ebe27e45bef 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -41,11 +41,12 @@ inline AsyncWrap::AsyncWrap(Environment* env, v8::Local argv[] = { v8::Int32::New(env->isolate(), provider), + v8::Integer::New(env->isolate(), get_uid()), Null(env->isolate()) }; if (parent != nullptr) - argv[1] = parent->object(); + argv[2] = parent->object(); v8::MaybeLocal ret = init_fn->Call(env->context(), object, ARRAY_SIZE(argv), argv); @@ -57,6 +58,22 @@ inline AsyncWrap::AsyncWrap(Environment* env, } +inline AsyncWrap::~AsyncWrap() { + if (!ran_init_callback()) + return; + + v8::Local fn = env()->async_hooks_destroy_function(); + if (!fn.IsEmpty()) { + v8::HandleScope scope(env()->isolate()); + v8::Local uid = v8::Integer::New(env()->isolate(), get_uid()); + v8::MaybeLocal ret = + fn->Call(env()->context(), v8::Null(env()->isolate()), 1, &uid); + if (ret.IsEmpty()) + FatalError("node::AsyncWrap::~AsyncWrap", "destroy hook threw"); + } +} + + inline bool AsyncWrap::ran_init_callback() const { return static_cast(bits_ & 1); } diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 767e76831c3d04..c9f5caad1e4ea8 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -131,6 +131,8 @@ static void SetupHooks(const FunctionCallbackInfo& args) { env->set_async_hooks_pre_function(args[1].As()); if (args[2]->IsFunction()) env->set_async_hooks_post_function(args[2].As()); + if (args[3]->IsFunction()) + env->set_async_hooks_destroy_function(args[3].As()); } @@ -156,6 +158,7 @@ static void Initialize(Local target, env->set_async_hooks_init_function(Local()); env->set_async_hooks_pre_function(Local()); env->set_async_hooks_post_function(Local()); + env->set_async_hooks_destroy_function(Local()); } diff --git a/src/async-wrap.h b/src/async-wrap.h index 5fbd2309383d87..5db29600bcd180 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -51,7 +51,7 @@ class AsyncWrap : public BaseObject { ProviderType provider, AsyncWrap* parent = nullptr); - inline virtual ~AsyncWrap() override = default; + inline virtual ~AsyncWrap(); inline ProviderType provider_type() const; diff --git a/src/env.h b/src/env.h index cdfd19c9740e31..93f7c47c9919a3 100644 --- a/src/env.h +++ b/src/env.h @@ -236,6 +236,7 @@ namespace node { V(async_hooks_init_function, v8::Function) \ V(async_hooks_pre_function, v8::Function) \ V(async_hooks_post_function, v8::Function) \ + V(async_hooks_destroy_function, v8::Function) \ V(binding_cache_object, v8::Object) \ V(buffer_constructor_function, v8::Function) \ V(buffer_prototype_object, v8::Object) \ diff --git a/test/parallel/test-async-wrap-disabled-propagate-parent.js b/test/parallel/test-async-wrap-disabled-propagate-parent.js index de36071524c4fe..70d82befe72a63 100644 --- a/test/parallel/test-async-wrap-disabled-propagate-parent.js +++ b/test/parallel/test-async-wrap-disabled-propagate-parent.js @@ -10,7 +10,7 @@ let cntr = 0; let server; let client; -function init(type, parent) { +function init(type, id, parent) { if (parent) { cntr++; // Cannot assert in init callback or will abort. diff --git a/test/parallel/test-async-wrap-propagate-parent.js b/test/parallel/test-async-wrap-propagate-parent.js index 8074b0062e0db2..beeb27ba7866a9 100644 --- a/test/parallel/test-async-wrap-propagate-parent.js +++ b/test/parallel/test-async-wrap-propagate-parent.js @@ -9,7 +9,7 @@ let cntr = 0; let server; let client; -function init(type, parent) { +function init(type, id, parent) { if (parent) { cntr++; // Cannot assert in init callback or will abort.