From 5493996a9131ddf792071d4effa9b8bc853e215f Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Sun, 24 Apr 2016 22:04:53 -0400 Subject: [PATCH] lib,src: throw on unhanded promise rejections --- lib/internal/process/promises.js | 13 ++++- node.gyp | 2 + src/env.h | 1 + src/node.cc | 83 +++++++++++++++++++++++++++- src/node_internals.h | 6 ++ src/track-promise.cc | 67 ++++++++++++++++++++++ src/track-promise.h | 31 +++++++++++ test/message/promise_fast_reject.js | 10 ++++ test/message/promise_fast_reject.out | 15 +++++ test/parallel/test-util-inspect.js | 3 + 10 files changed, 227 insertions(+), 4 deletions(-) create mode 100644 src/track-promise.cc create mode 100644 src/track-promise.h create mode 100644 test/message/promise_fast_reject.js create mode 100644 test/message/promise_fast_reject.out diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 22f1959784be9a..0bdf4d50c6882d 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -7,6 +7,8 @@ const pendingUnhandledRejections = []; exports.setup = setupPromises; function setupPromises(scheduleMicrotasks) { + const promiseInternals = {}; + process._setupPromises(function(event, promise, reason) { if (event === promiseRejectEvent.unhandled) unhandledRejection(promise, reason); @@ -14,7 +16,12 @@ function setupPromises(scheduleMicrotasks) { rejectionHandled(promise); else require('assert').fail(null, null, 'unexpected PromiseRejectEvent'); - }); + }, function getPromiseReason(data) { + var valueindex = data.indexOf('[[PromiseValue]]'); + if (valueindex > 0) { + return data[valueindex + 1]; + } + }, promiseInternals); function unhandledRejection(promise, reason) { hasBeenNotifiedProperty.set(promise, false); @@ -26,6 +33,7 @@ function setupPromises(scheduleMicrotasks) { if (hasBeenNotified !== undefined) { hasBeenNotifiedProperty.delete(promise); if (hasBeenNotified === true) { + promiseInternals.untrackPromise(promise); process.nextTick(function() { process.emit('rejectionHandled', promise); }); @@ -42,8 +50,7 @@ function setupPromises(scheduleMicrotasks) { if (hasBeenNotifiedProperty.get(promise) === false) { hasBeenNotifiedProperty.set(promise, true); if (!process.emit('unhandledRejection', reason, promise)) { - // Nobody is listening. - // TODO(petkaantonov) Take some default action, see #830 + promiseInternals.onPromiseGC(promise); } else { hadListeners = true; } diff --git a/node.gyp b/node.gyp index 0e9fe40c419af6..67fdb9b7276d20 100644 --- a/node.gyp +++ b/node.gyp @@ -152,6 +152,7 @@ 'src/stream_wrap.cc', 'src/tcp_wrap.cc', 'src/timer_wrap.cc', + 'src/track-promise.cc', 'src/tty_wrap.cc', 'src/process_wrap.cc', 'src/udp_wrap.cc', @@ -180,6 +181,7 @@ 'src/node_revert.h', 'src/node_i18n.h', 'src/pipe_wrap.h', + 'src/track-promise.h', 'src/tty_wrap.h', 'src/tcp_wrap.h', 'src/udp_wrap.h', diff --git a/src/env.h b/src/env.h index 9b117e1de0e391..a44a70dfc3cb78 100644 --- a/src/env.h +++ b/src/env.h @@ -271,6 +271,7 @@ namespace node { V(pipe_constructor_template, v8::FunctionTemplate) \ V(process_object, v8::Object) \ V(promise_reject_function, v8::Function) \ + V(promise_unhandled_rejection, v8::Function) \ V(push_values_to_array_function, v8::Function) \ V(script_context_constructor_template, v8::FunctionTemplate) \ V(script_data_constructor_function, v8::Function) \ diff --git a/src/node.cc b/src/node.cc index dd427c792e91ea..6953263363653c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -37,6 +37,7 @@ #include "req-wrap.h" #include "req-wrap-inl.h" #include "string_bytes.h" +#include "track-promise.h" #include "util.h" #include "uv.h" #include "libplatform/libplatform.h" @@ -104,6 +105,7 @@ using v8::Array; using v8::ArrayBuffer; using v8::Boolean; using v8::Context; +using v8::Debug; using v8::EscapableHandleScope; using v8::Exception; using v8::Function; @@ -118,10 +120,12 @@ using v8::Locker; using v8::MaybeLocal; using v8::Message; using v8::Name; +using v8::NativeWeakMap; using v8::Null; using v8::Number; using v8::Object; using v8::ObjectTemplate; +using v8::Persistent; using v8::Promise; using v8::PromiseRejectMessage; using v8::PropertyCallbackInfo; @@ -167,6 +171,10 @@ static const char* icu_data_dir = nullptr; // used by C++ modules as well bool no_deprecation = false; +// Unhandled promises tracking +Persistent* remainingURs; +Persistent* first_ur_key; + #if HAVE_OPENSSL && NODE_FIPS_MODE // used by crypto module bool enable_fips_crypto = false; @@ -1138,14 +1146,59 @@ void PromiseRejectCallback(PromiseRejectMessage message) { callback->Call(process, arraysize(args), args); } +void OnPromiseGC(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + CHECK(args[0]->IsObject()); + Local promise = args[0].As(); + + TrackPromise::New(env->isolate(), promise); + + HandleScope scope(env->isolate()); + Local l_remainingURs = + Local::New(env->isolate(), *remainingURs); + Local l_first_ur_key = + Local::New(env->isolate(), *first_ur_key); + if (!l_remainingURs->Has(l_first_ur_key)) { + l_remainingURs->Set(l_first_ur_key, promise); + } +} + +void UntrackPromise(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + CHECK(args[0]->IsObject()); + Local promise = args[0].As(); + + HandleScope scope(env->isolate()); + Local l_remainingURs = + Local::New(env->isolate(), *remainingURs); + Local l_first_ur_key = + Local::New(env->isolate(), *first_ur_key); + if (l_remainingURs->Get(l_first_ur_key) == promise) { + l_remainingURs->Delete(l_first_ur_key); + } +} + void SetupPromises(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); + HandleScope scope(isolate); + remainingURs = new Persistent(isolate, + NativeWeakMap::New(isolate)); + first_ur_key = new Persistent(isolate, Object::New(isolate)); + CHECK(args[0]->IsFunction()); + CHECK(args[1]->IsFunction()); + CHECK(args[2]->IsObject()); isolate->SetPromiseRejectCallback(PromiseRejectCallback); env->set_promise_reject_function(args[0].As()); + env->set_promise_unhandled_rejection(args[1].As()); + + env->SetMethod(args[2].As(), "onPromiseGC", OnPromiseGC); + env->SetMethod(args[2].As(), "untrackPromise", UntrackPromise); env->process_object()->Delete( env->context(), @@ -1574,8 +1627,26 @@ void AppendExceptionLine(Environment* env, PrintErrorString("\n%s", arrow); } +void ReportPromiseRejection(Isolate* isolate, Local object) { + Environment* env = Environment::GetCurrent(isolate); + Local fn = env->promise_unhandled_rejection(); + + if (!fn.IsEmpty()) { + HandleScope scope(isolate); + Local promise = object.As(); + Local internalProps = + Debug::GetInternalProperties(isolate, + promise).ToLocalChecked().As(); + + Local err = fn->Call(env->context(), Null(env->isolate()), 1, + &internalProps).ToLocalChecked(); + + ReportException(env, err, Exception::CreateMessage(isolate, err)); + } + exit(1); +} -static void ReportException(Environment* env, +void ReportException(Environment* env, Local er, Local message) { HandleScope scope(env->isolate()); @@ -4323,6 +4394,16 @@ static void StartNodeInstance(void* arg) { } while (more == true); } + HandleScope scope(isolate); + Local l_remainingURs = + Local::New(isolate, *remainingURs); + Local l_first_ur_key = + Local::New(env->isolate(), *first_ur_key); + if (l_remainingURs->Has(l_first_ur_key)) { + Local firstUR = l_remainingURs->Get(l_first_ur_key).As(); + ReportPromiseRejection(isolate, firstUR); + } + env->set_trace_sync_io(false); int exit_code = EmitExit(env); diff --git a/src/node_internals.h b/src/node_internals.h index 485ba18b3c742e..dba5da926778f7 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -128,6 +128,12 @@ constexpr size_t arraysize(const T(&)[N]) { return N; } bool IsExceptionDecorated(Environment* env, v8::Local er); +void ReportPromiseRejection(v8::Isolate* isolate, v8::Local object); + +void ReportException(Environment* env, + v8::Local er, + v8::Local message); + void AppendExceptionLine(Environment* env, v8::Local er, v8::Local message); diff --git a/src/track-promise.cc b/src/track-promise.cc new file mode 100644 index 00000000000000..17f9878cba66b7 --- /dev/null +++ b/src/track-promise.cc @@ -0,0 +1,67 @@ +#include "env.h" +#include "env-inl.h" +#include "node_internals.h" + +namespace node { + +using v8::Exception; +using v8::Function; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::Object; +using v8::Persistent; +using v8::Value; +using v8::WeakCallbackData; + +typedef void (*FreeCallback)(Local object, Local fn); + +class TrackPromise { + public: + static TrackPromise* New(Isolate* isolate, Local object); + inline Persistent* persistent(); + private: + static void WeakCallback(const WeakCallbackData&); + inline void WeakCallback(Isolate* isolate, Local object); + inline TrackPromise(Isolate* isolate, Local object); + ~TrackPromise(); + Persistent persistent_; +}; + + +TrackPromise* TrackPromise::New(Isolate* isolate, + Local object) { + return new TrackPromise(isolate, object); +} + + +Persistent* TrackPromise::persistent() { + return &persistent_; +} + + +TrackPromise::TrackPromise(Isolate* isolate, + Local object) + : persistent_(isolate, object) { + persistent_.SetWeak(this, WeakCallback); + persistent_.MarkIndependent(); +} + + +TrackPromise::~TrackPromise() { + persistent_.Reset(); +} + + +void TrackPromise::WeakCallback( + const WeakCallbackData& data) { + data.GetParameter()->WeakCallback(data.GetIsolate(), data.GetValue()); +} + + +void TrackPromise::WeakCallback(Isolate* isolate, Local object) { + node::ReportPromiseRejection(isolate, object); + delete this; +} + +} // namespace node diff --git a/src/track-promise.h b/src/track-promise.h new file mode 100644 index 00000000000000..755c0520305eb4 --- /dev/null +++ b/src/track-promise.h @@ -0,0 +1,31 @@ +#ifndef SRC_TRACK_PROMISE_H_ +#define SRC_TRACK_PROMISE_H_ + +#include "v8.h" + +namespace node { + +class Environment; + +class TrackPromise { + public: + TrackPromise(v8::Isolate* isolate, v8::Local object); + virtual ~TrackPromise(); + + static TrackPromise* New(v8::Isolate* isolate, + v8::Local object); + + inline v8::Persistent& persistent(); + + inline void WeakCallback( + const v8::WeakCallbackData& data); + + private: + inline void WeakCallback(v8::Isolate* isolate, v8::Local object); + + v8::Persistent persistent_; +}; + +} // namespace node + +#endif // SRC_TRACK_PROMISE_H_ diff --git a/test/message/promise_fast_reject.js b/test/message/promise_fast_reject.js new file mode 100644 index 00000000000000..25393c0a331350 --- /dev/null +++ b/test/message/promise_fast_reject.js @@ -0,0 +1,10 @@ +'use strict'; +require('../common'); + +new Promise(function(res, rej) { + consol.log('One'); +}); + +new Promise(function(res, rej) { + consol.log('Two'); +}); diff --git a/test/message/promise_fast_reject.out b/test/message/promise_fast_reject.out new file mode 100644 index 00000000000000..d4813429dfb67c --- /dev/null +++ b/test/message/promise_fast_reject.out @@ -0,0 +1,15 @@ +*test*message*promise_fast_reject.js:* + consol.log('One'); + ^ + +ReferenceError: consol is not defined + at *test*message*promise_fast_reject.js:*:* + at Object. (*test*message*promise_fast_reject.js:*:*) + at Module._compile (module.js:*:*) + at Object.Module._extensions..js (module.js:*:*) + at Module.load (module.js:*:*) + at tryModuleLoad (module.js:*:*) + at Function.Module._load (module.js:*:*) + at Function.Module.runMain (module.js:*:*) + at startup (node.js:*:*) + at node.js:*:* diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js index 115695db6a6487..dff5d9c27e9fc3 100644 --- a/test/parallel/test-util-inspect.js +++ b/test/parallel/test-util-inspect.js @@ -532,6 +532,9 @@ assert.equal(util.inspect(set, true), 'Set { \'foo\', [size]: 1, bar: 42 }'); // test Promise assert.equal(util.inspect(Promise.resolve(3)), 'Promise { 3 }'); +process.on('unhandledRejection', (message) => { + assert.strictEqual(3, message); +}); assert.equal(util.inspect(Promise.reject(3)), 'Promise { 3 }'); assert.equal(util.inspect(new Promise(function() {})), 'Promise { }'); var promise = Promise.resolve('foo');