From 4321206c5bc2aabaef8e7d22c45860bc6581d5c3 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 19 Jun 2017 23:21:33 -0600 Subject: [PATCH 1/5] build: add test-with-async-hooks The new test-with-async-hooks runs all normal tests (except async-hooks) with the environment variable NODE_TEST_WITH_ASYNC_HOOKS set. These extra checks do a minimum check to make sure async_hooks operates normally under all other tests. e.g. if init() or destroy() is called twice for the same id. Also move test "async-hooks" from CI_JS_SUITES into its own CI_ASYNC_HOOKS. Makes it cleaner to add, instead of supplying a massive list of tests that may change in the future. PR-URL: https://github.com/nodejs/node/pull/14208 Reviewed-By: James M Snell Reviewed-By: Refael Ackermann --- Makefile | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 825baa1ecb62a7..cd0bfd312224d2 100644 --- a/Makefile +++ b/Makefile @@ -202,6 +202,7 @@ test: all $(MAKE) build-addons-napi $(MAKE) cctest $(PYTHON) tools/test.py --mode=release -J \ + $(CI_ASYNC_HOOKS) \ $(CI_JS_SUITES) \ $(CI_NATIVE_SUITES) $(MAKE) lint @@ -334,7 +335,8 @@ test-all-valgrind: test-build $(PYTHON) tools/test.py --mode=debug,release --valgrind CI_NATIVE_SUITES := addons addons-napi -CI_JS_SUITES := abort async-hooks doctool inspector known_issues message parallel pseudo-tty sequential +CI_ASYNC_HOOKS := async-hooks +CI_JS_SUITES := abort doctool inspector known_issues message parallel pseudo-tty sequential # Build and test addons without building anything else test-ci-native: LOGLEVEL := info @@ -347,7 +349,7 @@ test-ci-native: | test/addons/.buildstamp test/addons-napi/.buildstamp test-ci-js: | clear-stalled $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ --mode=release --flaky-tests=$(FLAKY_TESTS) \ - $(TEST_CI_ARGS) $(CI_JS_SUITES) + $(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) # Clean up any leftover processes, error if found. ps awwx | grep Release/node | grep -v grep | cat @PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \ @@ -360,7 +362,7 @@ test-ci: | clear-stalled build-addons build-addons-napi out/Release/cctest --gtest_output=tap:cctest.tap $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ --mode=release --flaky-tests=$(FLAKY_TESTS) \ - $(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) + $(TEST_CI_ARGS) $(CI_ASYNC_HOOKS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) # Clean up any leftover processes, error if found. ps awwx | grep Release/node | grep -v grep | cat @PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \ @@ -434,6 +436,14 @@ test-timers-clean: test-async-hooks: $(PYTHON) tools/test.py --mode=release async-hooks +test-with-async-hooks: + $(MAKE) build-addons + $(MAKE) build-addons-napi + $(MAKE) cctest + NODE_TEST_WITH_ASYNC_HOOKS=1 $(PYTHON) tools/test.py --mode=release -J \ + $(CI_JS_SUITES) \ + $(CI_NATIVE_SUITES) + ifneq ("","$(wildcard deps/v8/tools/run-tests.py)") test-v8: v8 From 44dc449effdd607065f2bd112f16f4b795ea067c Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Fri, 16 Jun 2017 16:25:02 -0600 Subject: [PATCH 2/5] test: print resource stack on error When running tests with NODE_TEST_WITH_ASYNC_HOOKS and the same asyncId is detected twice print the stack traces of both init() calls. Also print if the resource is the same instance. PR-URL: https://github.com/nodejs/node/pull/14208 Reviewed-By: James M Snell Reviewed-By: Refael Ackermann --- test/common/index.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index c4f899c9ec5d15..278f9f6a781474 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -79,18 +79,21 @@ if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { if (destroyListList[id] !== undefined) { process._rawDebug(destroyListList[id]); process._rawDebug(); - throw new Error(`same id added twice (${id})`); + throw new Error(`same id added to destroy list twice (${id})`); } destroyListList[id] = new Error().stack; _queueDestroyAsyncId(id); }; require('async_hooks').createHook({ - init(id, ty, tr, h) { + init(id, ty, tr, r) { if (initHandles[id]) { + process._rawDebug( + `Is same resource: ${r === initHandles[id].resource}`); + process._rawDebug(`Previous stack:\n${initHandles[id].stack}\n`); throw new Error(`init called twice for same id (${id})`); } - initHandles[id] = h; + initHandles[id] = { resource: r, stack: new Error().stack.substr(6) }; }, before() { }, after() { }, From d28d20ccb7662e7ef83a925e60bd2c37151e970c Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 19 Jun 2017 23:13:00 -0600 Subject: [PATCH 3/5] test: skip test when checking async_hooks The test addons/async-hooks-promise depends on there being only one hook available. So skip it if NODE_TEST_WITH_ASYNC_HOOKS is set. PR-URL: https://github.com/nodejs/node/pull/14208 Reviewed-By: James M Snell Reviewed-By: Refael Ackermann --- test/addons/async-hooks-promise/test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/addons/async-hooks-promise/test.js b/test/addons/async-hooks-promise/test.js index b0af8806bd665f..e7923c9c57a6e9 100644 --- a/test/addons/async-hooks-promise/test.js +++ b/test/addons/async-hooks-promise/test.js @@ -5,6 +5,11 @@ const assert = require('assert'); const async_hooks = require('async_hooks'); const binding = require(`./build/${common.buildType}/binding`); +if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { + common.skip('cannot test with env var NODE_TEST_WITH_ASYNC_HOOKS'); + return; +} + // Baseline to make sure the internal field isn't being set. assert.strictEqual( binding.getPromiseField(Promise.resolve(1)), From e1eae3cf9f22f34d39f26ff717514d6aab31e184 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 14 Jun 2017 01:04:04 -0600 Subject: [PATCH 4/5] async_wrap: add constructor for PromiseWrap Another optional argument is about to be added to AsyncWrap. So instead of piling them on, create a separate constructor specifically for PromiseWrap since it's the only class that uses the "silent" argument. PR-URL: https://github.com/nodejs/node/pull/14208 Reviewed-By: James M Snell Reviewed-By: Refael Ackermann --- src/async-wrap.cc | 21 ++++++++++++++++++--- src/async-wrap.h | 7 +++++-- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 7ccc108d375925..c9e1286aade8d3 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -243,7 +243,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, silent) { + : AsyncWrap(env, object, silent) { MakeWeak(this); } size_t self_size() const override { return sizeof(*this); } @@ -573,8 +573,7 @@ void LoadAsyncWrapperInfo(Environment* env) { AsyncWrap::AsyncWrap(Environment* env, Local object, - ProviderType provider, - bool silent) + ProviderType provider) : BaseObject(env, object), provider_type_(provider) { CHECK_NE(provider, PROVIDER_NONE); @@ -583,6 +582,22 @@ AsyncWrap::AsyncWrap(Environment* env, // Shift provider value over to prevent id collision. persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider); + // Use AsyncReset() call to execute the init() callbacks. + AsyncReset(); +} + + +// This is specifically used by the PromiseWrap constructor. +AsyncWrap::AsyncWrap(Environment* env, + Local object, + bool silent) + : BaseObject(env, object), + provider_type_(PROVIDER_PROMISE) { + CHECK_GE(object->InternalFieldCount(), 1); + + // Shift provider value over to prevent id collision. + persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider_type_); + // Use AsyncReset() call to execute the init() callbacks. AsyncReset(silent); } diff --git a/src/async-wrap.h b/src/async-wrap.h index e860afb90502d7..0e0415da17e51c 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -94,8 +94,7 @@ class AsyncWrap : public BaseObject { AsyncWrap(Environment* env, v8::Local object, - ProviderType provider, - bool silent = false); + ProviderType provider); virtual ~AsyncWrap(); @@ -150,6 +149,10 @@ class AsyncWrap : public BaseObject { virtual size_t self_size() const = 0; private: + friend class PromiseWrap; + + // This is specifically used by the PromiseWrap constructor. + AsyncWrap(Environment* env, v8::Local promise, bool silent); inline AsyncWrap(); const ProviderType provider_type_; // Because the values may be Reset(), cannot be made const. From 2b9b46cd55835b33382dacb86f614cbcdc5f7b98 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 14 Jun 2017 01:27:02 -0600 Subject: [PATCH 5/5] async_wrap: allow user to pass execution_async_id Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: https://github.com/nodejs/node/pull/14208 Reviewed-By: James M Snell Reviewed-By: Refael Ackermann --- src/async-wrap.cc | 15 +++++++++------ src/async-wrap.h | 5 +++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index c9e1286aade8d3..a16d93888925c6 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -451,7 +451,8 @@ void AsyncWrap::ClearAsyncIdStack(const FunctionCallbackInfo& args) { void AsyncWrap::AsyncReset(const FunctionCallbackInfo& args) { AsyncWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - wrap->AsyncReset(); + double execution_async_id = args[0]->IsNumber() ? args[0]->NumberValue() : -1; + wrap->AsyncReset(execution_async_id); } @@ -573,7 +574,8 @@ void LoadAsyncWrapperInfo(Environment* env) { AsyncWrap::AsyncWrap(Environment* env, Local object, - ProviderType provider) + ProviderType provider, + double execution_async_id) : BaseObject(env, object), provider_type_(provider) { CHECK_NE(provider, PROVIDER_NONE); @@ -583,7 +585,7 @@ AsyncWrap::AsyncWrap(Environment* env, persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider); // Use AsyncReset() call to execute the init() callbacks. - AsyncReset(); + AsyncReset(execution_async_id); } @@ -599,7 +601,7 @@ AsyncWrap::AsyncWrap(Environment* env, persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider_type_); // Use AsyncReset() call to execute the init() callbacks. - AsyncReset(silent); + AsyncReset(-1, silent); } @@ -611,8 +613,9 @@ AsyncWrap::~AsyncWrap() { // Generalized call for both the constructor and for handles that are pooled // 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(bool silent) { - async_id_ = env()->new_async_id(); +void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { + async_id_ = + execution_async_id == -1 ? env()->new_async_id() : execution_async_id; trigger_async_id_ = env()->get_init_trigger_async_id(); if (silent) return; diff --git a/src/async-wrap.h b/src/async-wrap.h index 0e0415da17e51c..d24eb0d46d51b6 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -94,7 +94,8 @@ class AsyncWrap : public BaseObject { AsyncWrap(Environment* env, v8::Local object, - ProviderType provider); + ProviderType provider, + double execution_async_id = -1); virtual ~AsyncWrap(); @@ -132,7 +133,7 @@ class AsyncWrap : public BaseObject { inline double get_trigger_async_id() const; - void AsyncReset(bool silent = false); + void AsyncReset(double execution_async_id = -1, bool silent = false); // Only call these within a valid HandleScope. v8::MaybeLocal MakeCallback(const v8::Local cb,