From 7ce47698a21c38036f054a6291929a61056aaf61 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 19 Apr 2017 19:17:31 +0200 Subject: [PATCH 01/10] test: introduce `common.crashOnUnhandledRejection` --- test/common.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/common.js b/test/common.js index 6fe2d4520f6a08..1d45a32e3c45e7 100644 --- a/test/common.js +++ b/test/common.js @@ -675,3 +675,8 @@ exports.getArrayBufferViews = function getArrayBufferViews(buf) { } return out; }; + +// Crash the process on unhandled rejections. +exports.crashOnUnhandledRejection = function() { + process.on('unhandledRejection', (err) => setImmediate(() => { throw err; })); +}; From 0a07fb93f0d30f224e7251196f497000efd7d12c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 23 Apr 2017 18:03:24 -0700 Subject: [PATCH 02/10] [squash] use nextTick in crashOnUnhandledRejection --- test/common.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/common.js b/test/common.js index 1d45a32e3c45e7..3af9dda7f6c133 100644 --- a/test/common.js +++ b/test/common.js @@ -678,5 +678,6 @@ exports.getArrayBufferViews = function getArrayBufferViews(buf) { // Crash the process on unhandled rejections. exports.crashOnUnhandledRejection = function() { - process.on('unhandledRejection', (err) => setImmediate(() => { throw err; })); + process.on('unhandledRejection', + (err) => process.nextTick(() => { throw err; })); }; From fbe240a354d20af4510c327e14d20fe51fb9e81f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 21 Apr 2017 14:06:21 -0700 Subject: [PATCH 03/10] src: expose `node::AddPromiseHook` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expose `node::AddPromiseHook`, which wraps V8’s `SetPromiseHook` in a way that allows multiple hooks to be set up. --- src/env.cc | 16 ++++++++++++++++ src/env.h | 13 +++++++++++++ src/node.cc | 6 ++++++ src/node.h | 11 +++++++++++ 4 files changed, 46 insertions(+) diff --git a/src/env.cc b/src/env.cc index b44b435d4e3256..034625b375446c 100644 --- a/src/env.cc +++ b/src/env.cc @@ -188,4 +188,20 @@ void Environment::AtExit(void (*cb)(void* arg), void* arg) { at_exit_functions_.push_back(AtExitCallback{cb, arg}); } +void Environment::AddPromiseHook(promise_hook_func fn, void* arg) { + promise_hooks_.push_back(PromiseHookCallback{fn, arg}); + if (promise_hooks_.size() == 1) { + isolate_->SetPromiseHook(EnvPromiseHook); + } +} + +void Environment::EnvPromiseHook(v8::PromiseHookType type, + v8::Local promise, + v8::Local parent) { + Environment* env = Environment::GetCurrent(promise->CreationContext()); + for (const PromiseHookCallback& hook : env->promise_hooks_) { + hook.cb_(type, promise, parent, hook.arg_); + } +} + } // namespace node diff --git a/src/env.h b/src/env.h index 8b158728a9a261..abf5f44de052df 100644 --- a/src/env.h +++ b/src/env.h @@ -35,6 +35,7 @@ #include "util.h" #include "uv.h" #include "v8.h" +#include "node.h" #include #include @@ -572,6 +573,8 @@ class Environment { static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX; + void AddPromiseHook(promise_hook_func fn, void* arg); + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -620,6 +623,16 @@ class Environment { }; std::list at_exit_functions_; + struct PromiseHookCallback { + promise_hook_func cb_; + void* arg_; + }; + std::vector promise_hooks_; + + static void EnvPromiseHook(v8::PromiseHookType type, + v8::Local promise, + v8::Local parent); + #define V(PropertyName, TypeName) \ v8::Persistent PropertyName ## _; ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) diff --git a/src/node.cc b/src/node.cc index 748e1ee4f32e12..20eea1769aba10 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1232,6 +1232,12 @@ void SetupPromises(const FunctionCallbackInfo& args) { } // anonymous namespace +void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { + Environment* env = Environment::GetCurrent(isolate); + env->AddPromiseHook(fn, arg); +} + + Local MakeCallback(Environment* env, Local recv, const Local callback, diff --git a/src/node.h b/src/node.h index 4452b9d578bc1c..feb2c800798e8d 100644 --- a/src/node.h +++ b/src/node.h @@ -517,6 +517,17 @@ NODE_EXTERN void AtExit(void (*cb)(void* arg), void* arg = 0); */ NODE_EXTERN void AtExit(Environment* env, void (*cb)(void* arg), void* arg = 0); +typedef void (*promise_hook_func) (v8::PromiseHookType type, + v8::Local promise, + v8::Local parent, + void* arg); + +/* Registers an additional v8::PromiseHook wrapper. This API exists because V8 + * itself supports only a single PromiseHook. */ +NODE_EXTERN void AddPromiseHook(v8::Isolate* isolate, + promise_hook_func fn, + void* arg); + } // namespace node #endif // SRC_NODE_H_ From d0e4911b65d6a9163000cb1d7bafc6288b69f6d9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 18 Apr 2017 18:02:27 +0200 Subject: [PATCH 04/10] domain: support promises Fixes: https://github.com/nodejs/node/issues/10724 --- src/node.cc | 53 ++++++++++++++++++++++++++++ test/parallel/test-domain-promise.js | 12 +++++++ 2 files changed, 65 insertions(+) create mode 100644 test/parallel/test-domain-promise.js diff --git a/src/node.cc b/src/node.cc index 20eea1769aba10..daf37da83a9e6c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -142,6 +142,7 @@ using v8::Number; using v8::Object; using v8::ObjectTemplate; using v8::Promise; +using v8::PromiseHookType; using v8::PromiseRejectMessage; using v8::PropertyCallbackInfo; using v8::ScriptOrigin; @@ -1113,6 +1114,56 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) { } +void PromiseHook(PromiseHookType type, + Local promise, + Local parent) { + Environment* env = Environment::GetCurrent(Isolate::GetCurrent()); + Local context = env->context(); + + if (type == PromiseHookType::kResolve) return; + if (type == PromiseHookType::kInit && env->in_domain()) { + promise->Set(context, + env->domain_string(), + env->domain_array()->Get(0)).FromJust(); + return; + } + + // Loosely based on node::MakeCallback(). + Local domain_v = + promise->Get(context, env->domain_string()).ToLocalChecked(); + if (!domain_v->IsObject()) + return; + + Local domain = domain_v.As(); + if (domain->Get(context, env->disposed_string()) + .ToLocalChecked()->IsTrue()) { + return; + } + + if (type == PromiseHookType::kBefore) { + Local enter_v = + domain->Get(context, env->enter_string()).ToLocalChecked(); + if (enter_v->IsFunction()) { + if (enter_v.As()->Call(context, domain, 0, nullptr).IsEmpty()) { + FatalError("node::PromiseHook", + "domain enter callback threw, please report this " + "as a bug in Node.js"); + } + } + } else { + Local exit_v = + domain->Get(context, env->exit_string()).ToLocalChecked(); + if (exit_v->IsFunction()) { + if (exit_v.As()->Call(context, domain, 0, nullptr).IsEmpty()) { + FatalError("node::MakeCallback", + "domain exit callback threw, please report this " + "as a bug in Node.js"); + } + } + } +} + + void SetupDomainUse(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1152,6 +1203,8 @@ void SetupDomainUse(const FunctionCallbackInfo& args) { Local array_buffer = ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count); + env->isolate()->SetPromiseHook(PromiseHook); + args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, fields_count)); } diff --git a/test/parallel/test-domain-promise.js b/test/parallel/test-domain-promise.js new file mode 100644 index 00000000000000..e0d5ca4b0dd9e4 --- /dev/null +++ b/test/parallel/test-domain-promise.js @@ -0,0 +1,12 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const domain = require('domain'); + +const d = domain.create(); + +d.run(common.mustCall(() => { + Promise.resolve().then(common.mustCall(() => { + assert.strictEqual(process.domain, d); + })); +})); From 51a52ee8aa14d9dda98bd08d23102058501f1f08 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 19 Apr 2017 09:06:30 +0200 Subject: [PATCH 05/10] [squash] address review comments --- src/node.cc | 7 ++++--- test/parallel/test-domain-promise.js | 7 +++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/node.cc b/src/node.cc index daf37da83a9e6c..1caac04a84cb12 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1117,14 +1117,15 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) { void PromiseHook(PromiseHookType type, Local promise, Local parent) { - Environment* env = Environment::GetCurrent(Isolate::GetCurrent()); - Local context = env->context(); + Local context = promise->CreationContext(); + Environment* env = Environment::GetCurrent(context); if (type == PromiseHookType::kResolve) return; if (type == PromiseHookType::kInit && env->in_domain()) { promise->Set(context, env->domain_string(), - env->domain_array()->Get(0)).FromJust(); + env->domain_array()->Get(context, + 0).ToLocalChecked()).FromJust(); return; } diff --git a/test/parallel/test-domain-promise.js b/test/parallel/test-domain-promise.js index e0d5ca4b0dd9e4..4aad28062012da 100644 --- a/test/parallel/test-domain-promise.js +++ b/test/parallel/test-domain-promise.js @@ -2,6 +2,7 @@ const common = require('../common'); const assert = require('assert'); const domain = require('domain'); +const vm = require('vm'); const d = domain.create(); @@ -10,3 +11,9 @@ d.run(common.mustCall(() => { assert.strictEqual(process.domain, d); })); })); + +d.run(common.mustCall(() => { + vm.runInNewContext(`Promise.resolve().then(common.mustCall(() => { + assert.strictEqual(process.domain, d); + }));`, { common, assert, process, d }); +})); From f5a9aa57bfa3611e69d2bc4466e1f5ef3a05bf1a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 19 Apr 2017 19:48:55 +0200 Subject: [PATCH 06/10] [squash] one more test + doc `changes:` tag --- doc/api/domain.md | 7 +++++++ test/parallel/test-domain-promise.js | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/doc/api/domain.md b/doc/api/domain.md index 00cb149bbd9f40..7d3520bc3648f9 100644 --- a/doc/api/domain.md +++ b/doc/api/domain.md @@ -1,4 +1,11 @@ # Domain + > Stability: 0 - Deprecated diff --git a/test/parallel/test-domain-promise.js b/test/parallel/test-domain-promise.js index 4aad28062012da..594cfded933a42 100644 --- a/test/parallel/test-domain-promise.js +++ b/test/parallel/test-domain-promise.js @@ -12,6 +12,12 @@ d.run(common.mustCall(() => { })); })); +d.run(common.mustCall(() => { + Promise.resolve().then(() => {}).then(() => {}).then(common.mustCall(() => { + assert.strictEqual(process.domain, d); + })); +})); + d.run(common.mustCall(() => { vm.runInNewContext(`Promise.resolve().then(common.mustCall(() => { assert.strictEqual(process.domain, d); From 07823ee8ea94cab9081762440d098d27726b29d3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 21 Apr 2017 14:04:55 -0700 Subject: [PATCH 07/10] [squash] use crashOnUncaughtException in test --- test/parallel/test-domain-promise.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/parallel/test-domain-promise.js b/test/parallel/test-domain-promise.js index 594cfded933a42..125f1765d22ce8 100644 --- a/test/parallel/test-domain-promise.js +++ b/test/parallel/test-domain-promise.js @@ -4,6 +4,8 @@ const assert = require('assert'); const domain = require('domain'); const vm = require('vm'); +common.crashOnUnhandledRejection(); + const d = domain.create(); d.run(common.mustCall(() => { From bc9692d8222d7cb26c2128de1ae5203ab70d7b98 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 21 Apr 2017 14:07:43 -0700 Subject: [PATCH 08/10] [squash] use AddPromiseHook for domain promise handling --- src/node.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/node.cc b/src/node.cc index 1caac04a84cb12..5a47b083a8b82a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1114,11 +1114,12 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) { } -void PromiseHook(PromiseHookType type, - Local promise, - Local parent) { - Local context = promise->CreationContext(); - Environment* env = Environment::GetCurrent(context); +void DomainPromiseHook(PromiseHookType type, + Local promise, + Local parent, + void* arg) { + Environment* env = static_cast(arg); + Local context = env->context(); if (type == PromiseHookType::kResolve) return; if (type == PromiseHookType::kInit && env->in_domain()) { @@ -1204,11 +1205,12 @@ void SetupDomainUse(const FunctionCallbackInfo& args) { Local array_buffer = ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count); - env->isolate()->SetPromiseHook(PromiseHook); + env->AddPromiseHook(DomainPromiseHook, static_cast(env)); args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, fields_count)); } + void RunMicrotasks(const FunctionCallbackInfo& args) { args.GetIsolate()->RunMicrotasks(); } From ddd7c476290db8a527fdaac5bdf9d4868e4481b2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 21 Apr 2017 14:32:41 -0700 Subject: [PATCH 09/10] [squash] more docs and tests --- doc/api/domain.md | 44 +++++++++ test/parallel/test-domain-promise.js | 129 ++++++++++++++++++++++++--- 2 files changed, 159 insertions(+), 14 deletions(-) diff --git a/doc/api/domain.md b/doc/api/domain.md index 7d3520bc3648f9..c70b932681bcc9 100644 --- a/doc/api/domain.md +++ b/doc/api/domain.md @@ -451,6 +451,50 @@ d.run(() => { In this example, the `d.on('error')` handler will be triggered, rather than crashing the program. +## Domains and Promises + +As of Node REPLACEME, the handlers of Promises are run inside the domain in +which the call to `.then` or `.catch` itself was made: + +```js +const d1 = domain.create(); +const d2 = domain.create(); + +let p; +d1.run(() => { + p = Promise.resolve(42); +}); + +d2.run(() => { + p.then((v) => { + // running in d2 + }); +}); +``` + +If you need to run that callback in a specific domain, you can use +[`domain.bind(callback)`][]: + +```js +const d1 = domain.create(); +const d2 = domain.create(); + +let p; +d1.run(() => { + p = Promise.resolve(42); +}); + +d2.run(() => { + p.then(p.domain.bind((v) => { + // running in d1 + })); +}); +``` + +Note that domains will not interfere with the error handling mechanisms for +Promises, i.e. no `error` event will be emitted for unhandled Promise +rejections. + [`domain.add(emitter)`]: #domain_domain_add_emitter [`domain.bind(callback)`]: #domain_domain_bind_callback [`domain.dispose()`]: #domain_domain_dispose diff --git a/test/parallel/test-domain-promise.js b/test/parallel/test-domain-promise.js index 125f1765d22ce8..8bae75eb63b76a 100644 --- a/test/parallel/test-domain-promise.js +++ b/test/parallel/test-domain-promise.js @@ -2,26 +2,127 @@ const common = require('../common'); const assert = require('assert'); const domain = require('domain'); +const fs = require('fs'); const vm = require('vm'); common.crashOnUnhandledRejection(); -const d = domain.create(); +{ + const d = domain.create(); -d.run(common.mustCall(() => { - Promise.resolve().then(common.mustCall(() => { - assert.strictEqual(process.domain, d); + d.run(common.mustCall(() => { + Promise.resolve().then(common.mustCall(() => { + assert.strictEqual(process.domain, d); + })); })); -})); +} -d.run(common.mustCall(() => { - Promise.resolve().then(() => {}).then(() => {}).then(common.mustCall(() => { - assert.strictEqual(process.domain, d); +{ + const d = domain.create(); + + d.run(common.mustCall(() => { + Promise.resolve().then(() => {}).then(() => {}).then(common.mustCall(() => { + assert.strictEqual(process.domain, d); + })); + })); +} + +{ + const d = domain.create(); + + d.run(common.mustCall(() => { + vm.runInNewContext(`Promise.resolve().then(common.mustCall(() => { + assert.strictEqual(process.domain, d); + }));`, { common, assert, process, d }); })); -})); +} -d.run(common.mustCall(() => { - vm.runInNewContext(`Promise.resolve().then(common.mustCall(() => { - assert.strictEqual(process.domain, d); - }));`, { common, assert, process, d }); -})); +{ + const d1 = domain.create(); + const d2 = domain.create(); + let p; + d1.run(common.mustCall(() => { + p = Promise.resolve(42); + })); + + d2.run(common.mustCall(() => { + p.then(common.mustCall((v) => { + assert.strictEqual(process.domain, d2); + assert.strictEqual(p.domain, d1); + })); + })); +} + +{ + const d1 = domain.create(); + const d2 = domain.create(); + let p; + d1.run(common.mustCall(() => { + p = Promise.resolve(42); + })); + + d2.run(common.mustCall(() => { + p.then(p.domain.bind(common.mustCall((v) => { + assert.strictEqual(process.domain, d1); + assert.strictEqual(p.domain, d1); + }))); + })); +} + +{ + const d1 = domain.create(); + const d2 = domain.create(); + let p; + d1.run(common.mustCall(() => { + p = Promise.resolve(42); + })); + + d1.run(common.mustCall(() => { + d2.run(common.mustCall(() => { + p.then(common.mustCall((v) => { + assert.strictEqual(process.domain, d2); + assert.strictEqual(p.domain, d1); + })); + })); + })); +} + +{ + const d1 = domain.create(); + const d2 = domain.create(); + let p; + d1.run(common.mustCall(() => { + p = Promise.reject(new Error('foobar')); + })); + + d2.run(common.mustCall(() => { + p.catch(common.mustCall((v) => { + assert.strictEqual(process.domain, d2); + assert.strictEqual(p.domain, d1); + })); + })); +} + +{ + const d = domain.create(); + + d.run(common.mustCall(() => { + Promise.resolve().then(common.mustCall(() => { + setTimeout(common.mustCall(() => { + assert.strictEqual(process.domain, d); + }), 0); + })); + })); +} + +{ + const d = domain.create(); + + d.run(common.mustCall(() => { + Promise.resolve().then(common.mustCall(() => { + fs.readFile(__filename, common.mustCall(() => { + assert.strictEqual(process.domain, d); + })); + })); + })); +} From a9c2078a60bc3012dc6156df19772697a56a2517 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 21 Apr 2017 16:50:28 -0700 Subject: [PATCH 10/10] [squash] avoid personal pronoun --- doc/api/domain.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/doc/api/domain.md b/doc/api/domain.md index c70b932681bcc9..b85e6096b23ebf 100644 --- a/doc/api/domain.md +++ b/doc/api/domain.md @@ -472,8 +472,7 @@ d2.run(() => { }); ``` -If you need to run that callback in a specific domain, you can use -[`domain.bind(callback)`][]: +A callback may be bound to a specific domain using [`domain.bind(callback)`][]: ```js const d1 = domain.create();