From 1f9635d8f860a758cfd295504db16b02a22e51df Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Mon, 25 Apr 2016 13:17:08 -0400 Subject: [PATCH 1/7] lib,src: throw on unhanded promise rejections Refs: https://github.com/nodejs/node/pull/5292 Refs: https://github.com/nodejs/promises/issues/26 Refs: https://github.com/nodejs/node/pull/6355 PR-URL: https://github.com/nodejs/node/pull/6375 --- .../request/node_modules/form-data/Readme.md | 217 ------------------ lib/internal/bootstrap_node.js | 4 +- lib/internal/process/promises.js | 52 +---- node.gyp | 2 + src/env.h | 6 +- src/node.cc | 123 +++++++++- src/node_internals.h | 13 ++ src/track-promise.cc | 63 +++++ src/track-promise.h | 31 +++ test/message/promise_fast_handled_reject.js | 26 +++ test/message/promise_fast_handled_reject.out | 16 ++ test/message/promise_fast_reject.js | 18 ++ test/message/promise_fast_reject.out | 16 ++ test/message/promise_reject.js | 27 +++ test/message/promise_reject.out | 16 ++ .../test-promises-gc-before-handled.js | 28 +++ test/parallel/test-promises-handled-reject.js | 19 ++ 17 files changed, 406 insertions(+), 271 deletions(-) delete mode 100644 deps/npm/node_modules/request/node_modules/form-data/Readme.md create mode 100644 src/track-promise.cc create mode 100644 src/track-promise.h create mode 100644 test/message/promise_fast_handled_reject.js create mode 100644 test/message/promise_fast_handled_reject.out create mode 100644 test/message/promise_fast_reject.js create mode 100644 test/message/promise_fast_reject.out create mode 100644 test/message/promise_reject.js create mode 100644 test/message/promise_reject.out create mode 100644 test/parallel/test-promises-gc-before-handled.js create mode 100644 test/parallel/test-promises-handled-reject.js diff --git a/deps/npm/node_modules/request/node_modules/form-data/Readme.md b/deps/npm/node_modules/request/node_modules/form-data/Readme.md deleted file mode 100644 index 642a9d14a800b7..00000000000000 --- a/deps/npm/node_modules/request/node_modules/form-data/Readme.md +++ /dev/null @@ -1,217 +0,0 @@ -# Form-Data [![NPM Module](https://img.shields.io/npm/v/form-data.svg)](https://www.npmjs.com/package/form-data) [![Join the chat at https://gitter.im/form-data/form-data](http://form-data.github.io/images/gitterbadge.svg)](https://gitter.im/form-data/form-data) - -A library to create readable ```"multipart/form-data"``` streams. Can be used to submit forms and file uploads to other web applications. - -The API of this library is inspired by the [XMLHttpRequest-2 FormData Interface][xhr2-fd]. - -[xhr2-fd]: http://dev.w3.org/2006/webapi/XMLHttpRequest-2/Overview.html#the-formdata-interface - -[![Linux Build](https://img.shields.io/travis/form-data/form-data/v2.1.2.svg?label=linux:0.12-6.x)](https://travis-ci.org/form-data/form-data) -[![MacOS Build](https://img.shields.io/travis/form-data/form-data/v2.1.2.svg?label=macos:0.12-6.x)](https://travis-ci.org/form-data/form-data) -[![Windows Build](https://img.shields.io/appveyor/ci/alexindigo/form-data/v2.1.2.svg?label=windows:0.12-6.x)](https://ci.appveyor.com/project/alexindigo/form-data) - -[![Coverage Status](https://img.shields.io/coveralls/form-data/form-data/v2.1.2.svg?label=code+coverage)](https://coveralls.io/github/form-data/form-data?branch=master) -[![Dependency Status](https://img.shields.io/david/form-data/form-data.svg)](https://david-dm.org/form-data/form-data) -[![bitHound Overall Score](https://www.bithound.io/github/form-data/form-data/badges/score.svg)](https://www.bithound.io/github/form-data/form-data) - -## Install - -``` -npm install --save form-data -``` - -## Usage - -In this example we are constructing a form with 3 fields that contain a string, -a buffer and a file stream. - -``` javascript -var FormData = require('form-data'); -var fs = require('fs'); - -var form = new FormData(); -form.append('my_field', 'my value'); -form.append('my_buffer', new Buffer(10)); -form.append('my_file', fs.createReadStream('/foo/bar.jpg')); -``` - -Also you can use http-response stream: - -``` javascript -var FormData = require('form-data'); -var http = require('http'); - -var form = new FormData(); - -http.request('http://nodejs.org/images/logo.png', function(response) { - form.append('my_field', 'my value'); - form.append('my_buffer', new Buffer(10)); - form.append('my_logo', response); -}); -``` - -Or @mikeal's [request](https://github.com/request/request) stream: - -``` javascript -var FormData = require('form-data'); -var request = require('request'); - -var form = new FormData(); - -form.append('my_field', 'my value'); -form.append('my_buffer', new Buffer(10)); -form.append('my_logo', request('http://nodejs.org/images/logo.png')); -``` - -In order to submit this form to a web application, call ```submit(url, [callback])``` method: - -``` javascript -form.submit('http://example.org/', function(err, res) { - // res – response object (http.IncomingMessage) // - res.resume(); -}); - -``` - -For more advanced request manipulations ```submit()``` method returns ```http.ClientRequest``` object, or you can choose from one of the alternative submission methods. - -### Alternative submission methods - -You can use node's http client interface: - -``` javascript -var http = require('http'); - -var request = http.request({ - method: 'post', - host: 'example.org', - path: '/upload', - headers: form.getHeaders() -}); - -form.pipe(request); - -request.on('response', function(res) { - console.log(res.statusCode); -}); -``` - -Or if you would prefer the `'Content-Length'` header to be set for you: - -``` javascript -form.submit('example.org/upload', function(err, res) { - console.log(res.statusCode); -}); -``` - -To use custom headers and pre-known length in parts: - -``` javascript -var CRLF = '\r\n'; -var form = new FormData(); - -var options = { - header: CRLF + '--' + form.getBoundary() + CRLF + 'X-Custom-Header: 123' + CRLF + CRLF, - knownLength: 1 -}; - -form.append('my_buffer', buffer, options); - -form.submit('http://example.com/', function(err, res) { - if (err) throw err; - console.log('Done'); -}); -``` - -Form-Data can recognize and fetch all the required information from common types of streams (```fs.readStream```, ```http.response``` and ```mikeal's request```), for some other types of streams you'd need to provide "file"-related information manually: - -``` javascript -someModule.stream(function(err, stdout, stderr) { - if (err) throw err; - - var form = new FormData(); - - form.append('file', stdout, { - filename: 'unicycle.jpg', - contentType: 'image/jpg', - knownLength: 19806 - }); - - form.submit('http://example.com/', function(err, res) { - if (err) throw err; - console.log('Done'); - }); -}); -``` - -For edge cases, like POST request to URL with query string or to pass HTTP auth credentials, object can be passed to `form.submit()` as first parameter: - -``` javascript -form.submit({ - host: 'example.com', - path: '/probably.php?extra=params', - auth: 'username:password' -}, function(err, res) { - console.log(res.statusCode); -}); -``` - -In case you need to also send custom HTTP headers with the POST request, you can use the `headers` key in first parameter of `form.submit()`: - -``` javascript -form.submit({ - host: 'example.com', - path: '/surelynot.php', - headers: {'x-test-header': 'test-header-value'} -}, function(err, res) { - console.log(res.statusCode); -}); -``` - -### Integration with other libraries - -#### Request - -Form submission using [request](https://github.com/request/request): - -```javascript -var formData = { - my_field: 'my_value', - my_file: fs.createReadStream(__dirname + '/unicycle.jpg'), -}; - -request.post({url:'http://service.com/upload', formData: formData}, function(err, httpResponse, body) { - if (err) { - return console.error('upload failed:', err); - } - console.log('Upload successful! Server responded with:', body); -}); -``` - -For more details see [request readme](https://github.com/request/request#multipartform-data-multipart-form-uploads). - -#### node-fetch - -You can also submit a form using [node-fetch](https://github.com/bitinn/node-fetch): - -```javascript -var form = new FormData(); - -form.append('a', 1); - -fetch('http://example.com', { method: 'POST', body: form }) - .then(function(res) { - return res.json(); - }).then(function(json) { - console.log(json); - }); -``` - -## Notes - -- ```getLengthSync()``` method DOESN'T calculate length for streams, use ```knownLength``` options as workaround. -- Starting version `2.x` FormData has dropped support for `node@0.10.x`. - -## License - -Form-Data is released under the [MIT](License) license. diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 038e71502bdb1a..105f5718800bda 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -304,13 +304,13 @@ function setupProcessFatal() { - process._fatalException = function(er) { + process._fatalException = function(er, fromPromise) { var caught; if (process.domain && process.domain._errorHandler) caught = process.domain._errorHandler(er) || caught; - if (!caught) + if (!caught && !fromPromise) caught = process.emit('uncaughtException', er); // If someone handled it, then great. otherwise, die in C++ land diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 0e382d11d5523b..f8399cfb3e3daa 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -2,18 +2,13 @@ const promiseRejectEvent = process._promiseRejectEvent; const hasBeenNotifiedProperty = new WeakMap(); -const promiseToGuidProperty = new WeakMap(); const pendingUnhandledRejections = []; -let lastPromiseId = 1; exports.setup = setupPromises; -function getAsynchronousRejectionWarningObject(uid) { - return new Error('Promise rejection was handled ' + - `asynchronously (rejection id: ${uid})`); -} - function setupPromises(scheduleMicrotasks) { + const promiseInternals = {}; + process._setupPromises(function(event, promise, reason) { if (event === promiseRejectEvent.unhandled) unhandledRejection(promise, reason); @@ -21,11 +16,12 @@ function setupPromises(scheduleMicrotasks) { rejectionHandled(promise); else require('assert').fail(null, null, 'unexpected PromiseRejectEvent'); - }); + }, function getPromiseReason(data) { + return data[data.indexOf('[[PromiseValue]]') + 1]; + }, promiseInternals); function unhandledRejection(promise, reason) { hasBeenNotifiedProperty.set(promise, false); - promiseToGuidProperty.set(promise, lastPromiseId++); addPendingUnhandledRejection(promise, reason); } @@ -33,47 +29,16 @@ function setupPromises(scheduleMicrotasks) { const hasBeenNotified = hasBeenNotifiedProperty.get(promise); if (hasBeenNotified !== undefined) { hasBeenNotifiedProperty.delete(promise); - const uid = promiseToGuidProperty.get(promise); - promiseToGuidProperty.delete(promise); if (hasBeenNotified === true) { - let warning = null; - if (!process.listenerCount('rejectionHandled')) { - // Generate the warning object early to get a good stack trace. - warning = getAsynchronousRejectionWarningObject(uid); - } + promiseInternals.untrackPromise(promise); process.nextTick(function() { - if (!process.emit('rejectionHandled', promise)) { - if (warning === null) - warning = getAsynchronousRejectionWarningObject(uid); - warning.name = 'PromiseRejectionHandledWarning'; - warning.id = uid; - process.emitWarning(warning); - } + process.emit('rejectionHandled', promise); }); } } } - function emitWarning(uid, reason) { - const warning = new Error('Unhandled promise rejection ' + - `(rejection id: ${uid}): ${reason}`); - warning.name = 'UnhandledPromiseRejectionWarning'; - warning.id = uid; - if (reason instanceof Error) { - warning.stack = reason.stack; - } - process.emitWarning(warning); - if (!deprecationWarned) { - deprecationWarned = true; - process.emitWarning( - 'Unhandled promise rejections are deprecated. In the future, ' + - 'promise rejections that are not handled will terminate the ' + - 'Node.js process with a non-zero exit code.', - 'DeprecationWarning', 'DEP0018'); - } - } - var deprecationWarned = false; function emitPendingUnhandledRejections() { let hadListeners = false; while (pendingUnhandledRejections.length > 0) { @@ -81,9 +46,8 @@ function setupPromises(scheduleMicrotasks) { const reason = pendingUnhandledRejections.shift(); if (hasBeenNotifiedProperty.get(promise) === false) { hasBeenNotifiedProperty.set(promise, true); - const uid = promiseToGuidProperty.get(promise); if (!process.emit('unhandledRejection', reason, promise)) { - emitWarning(uid, reason); + promiseInternals.trackPromise(promise); } else { hadListeners = true; } diff --git a/node.gyp b/node.gyp index 7a66f283d685d8..c8a15b8f1ca4f5 100644 --- a/node.gyp +++ b/node.gyp @@ -196,6 +196,7 @@ 'src/stream_wrap.cc', 'src/tcp_wrap.cc', 'src/timer_wrap.cc', + 'src/track-promise.cc', 'src/tracing/agent.cc', 'src/tracing/node_trace_buffer.cc', 'src/tracing/node_trace_writer.cc', @@ -230,6 +231,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 abf5f44de052df..f7482d386b281f 100644 --- a/src/env.h +++ b/src/env.h @@ -248,6 +248,7 @@ namespace node { V(zero_return_string, "ZERO_RETURN") \ #define ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) \ + V(array_from, v8::Function) \ V(as_external, v8::External) \ V(async_hooks_destroy_function, v8::Function) \ V(async_hooks_init_function, v8::Function) \ @@ -264,7 +265,10 @@ namespace node { V(module_load_list_array, v8::Array) \ V(pipe_constructor_template, v8::FunctionTemplate) \ V(process_object, v8::Object) \ - V(promise_reject_function, v8::Function) \ + V(promise_unhandled_rejection_function, v8::Function) \ + V(promise_unhandled_rejection, v8::Function) \ + V(promise_unhandled_reject_map, v8::NativeWeakMap) \ + V(promise_unhandled_reject_keys, v8::Set) \ 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 ed46dd3cb3a2f7..ffd2e9b032af4a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -59,6 +59,7 @@ #include "req-wrap-inl.h" #include "string_bytes.h" #include "tracing/agent.h" +#include "track-promise.h" #include "util.h" #include "uv.h" #if NODE_USE_V8_PLATFORM @@ -123,6 +124,7 @@ using v8::Array; using v8::ArrayBuffer; using v8::Boolean; using v8::Context; +using v8::Debug; using v8::EscapableHandleScope; using v8::Exception; using v8::Float64Array; @@ -138,6 +140,7 @@ using v8::MaybeLocal; using v8::Message; using v8::Name; using v8::NamedPropertyHandlerConfiguration; +using v8::NativeWeakMap; using v8::Null; using v8::Number; using v8::Object; @@ -148,6 +151,7 @@ using v8::PromiseRejectMessage; using v8::PropertyCallbackInfo; using v8::ScriptOrigin; using v8::SealHandleScope; +using v8::Set; using v8::String; using v8::TryCatch; using v8::Uint32Array; @@ -1261,7 +1265,7 @@ void PromiseRejectCallback(PromiseRejectMessage message) { Local event = Integer::New(isolate, message.GetEvent()); Environment* env = Environment::GetCurrent(isolate); - Local callback = env->promise_reject_function(); + Local callback = env->promise_unhandled_rejection_function(); if (value.IsEmpty()) value = Undefined(isolate); @@ -1272,14 +1276,78 @@ void PromiseRejectCallback(PromiseRejectMessage message) { callback->Call(process, arraysize(args), args); } +Local GetPromiseReason(Environment* env, Local promise) { + Local fn = env->promise_unhandled_rejection(); + + Local internal_props = + Debug::GetInternalProperties(env->isolate(), + promise).ToLocalChecked().As(); + + // If fn is empty we'll almost certainly have to panic anyways + return fn->Call(env->context(), Null(env->isolate()), 1, + &internal_props).ToLocalChecked(); +} + +void TrackPromise(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + CHECK(args[0]->IsObject()); + Local promise = args[0].As(); + + TrackPromise::New(env->isolate(), promise); + + Local promise_value = GetPromiseReason(env, promise); + Local unhandled_reject_map = + env->promise_unhandled_reject_map(); + Local unhandled_reject_keys = + env->promise_unhandled_reject_keys(); + + if (unhandled_reject_keys->Size() > 1000) { + return; + } + + if (!unhandled_reject_map->Has(promise_value) && + !promise_value->IsUndefined()) { + unhandled_reject_map->Set(promise_value, promise); + CHECK(!unhandled_reject_keys->Add(env->context(), promise_value).IsEmpty()); + } +} + +void UntrackPromise(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + CHECK(args[0]->IsObject()); + Local promise = args[0].As(); + + Local err = GetPromiseReason(env, promise); + Local unhandled_reject_map = + env->promise_unhandled_reject_map(); + Local unhandled_reject_keys = + env->promise_unhandled_reject_keys(); + + if (unhandled_reject_keys->Has(env->context(), err).IsJust()) { + CHECK(unhandled_reject_keys->Delete(env->context(), err).IsJust()); + unhandled_reject_map->Delete(err); + } +} + void SetupPromises(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); + env->set_promise_unhandled_reject_map(NativeWeakMap::New(isolate)); + env->set_promise_unhandled_reject_keys(Set::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_function(args[0].As()); + env->set_promise_unhandled_rejection(args[1].As()); + + env->SetMethod(args[2].As(), "trackPromise", TrackPromise); + env->SetMethod(args[2].As(), "untrackPromise", UntrackPromise); env->process_object()->Delete( env->context(), @@ -1719,10 +1787,9 @@ void AppendExceptionLine(Environment* env, arrow_str).FromMaybe(false)); } - -static void ReportException(Environment* env, - Local er, - Local message) { +void ReportException(Environment* env, + Local er, + Local message) { HandleScope scope(env->isolate()); AppendExceptionLine(env, er, message, FATAL_ERROR); @@ -2615,6 +2682,14 @@ NO_RETURN void FatalError(const char* location, const char* message) { void FatalException(Isolate* isolate, Local error, Local message) { + InternalFatalException(isolate, error, message, false); +} + + +void InternalFatalException(Isolate* isolate, + Local error, + Local message, + bool from_promise) { HandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); @@ -2637,9 +2712,12 @@ void FatalException(Isolate* isolate, // Do not call FatalException when _fatalException handler throws fatal_try_catch.SetVerbose(false); + Local argv[2] = { error, + Boolean::New(env->isolate(), from_promise) }; + // this will return true if the JS layer handled it, false otherwise Local caught = - fatal_exception_function->Call(process_object, 1, &error); + fatal_exception_function->Call(process_object, 2, argv); if (fatal_try_catch.HasCaught()) { // the fatal exception function threw, so we must exit @@ -3562,6 +3640,12 @@ void LoadEnvironment(Environment* env) { // Add a reference to the global object Local global = env->context()->Global(); + Local js_array_object = global->Get( + FIXED_ONE_BYTE_STRING(env->isolate(), "Array")).As(); + Local js_array_from_function = js_array_object->Get( + FIXED_ONE_BYTE_STRING(env->isolate(), "from")).As(); + env->set_array_from(js_array_from_function); + #if defined HAVE_DTRACE || defined HAVE_ETW InitDTrace(env, global); #endif @@ -4577,6 +4661,31 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, } while (more == true); } + Local promise_keys_set = + env.promise_unhandled_reject_keys().As(); + Local convert = env.array_from(); + Local ret = convert->Call(env.context(), + Null(env.isolate()), 1, &promise_keys_set).ToLocalChecked(); + Local promise_keys = ret.As(); + uint32_t key_count = promise_keys->Length(); + Local unhandled_reject_map = + env.promise_unhandled_reject_map(); + + for (uint32_t key_iter = 0; key_iter < key_count; key_iter++) { + Local key = promise_keys->Get(env.context(), + key_iter).ToLocalChecked(); + + if (unhandled_reject_map->Has(key)) { + Local promise = unhandled_reject_map->Get(key); + Local err = GetPromiseReason(&env, promise); + Local message = Exception::CreateMessage(isolate, err); + + // XXX(Fishrock123): Should this just call ReportException and + // set exit_code = 1 instead? + InternalFatalException(isolate, err, message, true); + } + } + env.set_trace_sync_io(false); const int exit_code = EmitExit(&env); diff --git a/src/node_internals.h b/src/node_internals.h index e07cb9d6d39a41..9b926c6e7fe038 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -163,6 +163,19 @@ constexpr size_t arraysize(const T(&)[N]) { return N; } bool IsExceptionDecorated(Environment* env, v8::Local er); enum ErrorHandlingMode { FATAL_ERROR, CONTEXTIFY_ERROR }; + +v8::Local GetPromiseReason(Environment* env, + v8::Local promise); + +void InternalFatalException(v8::Isolate* isolate, + v8::Local error, + v8::Local message, + bool from_promise); + +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..896be59c923e26 --- /dev/null +++ b/src/track-promise.cc @@ -0,0 +1,63 @@ +#include "track-promise.h" +#include "env.h" +#include "env-inl.h" +#include "node_internals.h" + +namespace node { + +using v8::Exception; +using v8::Function; +using v8::Isolate; +using v8::Local; +using v8::Message; +using v8::Object; +using v8::Persistent; +using v8::Value; +using v8::WeakCallbackInfo; +using v8::WeakCallbackType; + +typedef void (*FreeCallback)(Local object, Local fn); + + +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, WeakCallbackType::kFinalizer); + persistent_.MarkIndependent(); +} + + +TrackPromise::~TrackPromise() { + persistent_.Reset(); +} + + +void TrackPromise::WeakCallback( + const WeakCallbackInfo& data) { + data.GetParameter()->WeakCallback(data.GetIsolate()); +} + + +void TrackPromise::WeakCallback(Isolate* isolate) { + Environment* env = Environment::GetCurrent(isolate); + + Local promise = persistent_.Get(isolate); + Local err = node::GetPromiseReason(env, promise); + Local message = Exception::CreateMessage(isolate, err); + + node::InternalFatalException(isolate, err, message, true); + delete this; +} + +} // namespace node diff --git a/src/track-promise.h b/src/track-promise.h new file mode 100644 index 00000000000000..43f7b006eddaad --- /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(); + + static inline void WeakCallback( + const v8::WeakCallbackInfo& data); + + private: + inline void WeakCallback(v8::Isolate* isolate); + + v8::Persistent persistent_; +}; + +} // namespace node + +#endif // SRC_TRACK_PROMISE_H_ diff --git a/test/message/promise_fast_handled_reject.js b/test/message/promise_fast_handled_reject.js new file mode 100644 index 00000000000000..3925e63ae7c74e --- /dev/null +++ b/test/message/promise_fast_handled_reject.js @@ -0,0 +1,26 @@ +'use strict'; +const common = require('../common'); + +const p1 = new Promise((res, rej) => { + consol.log('One'); // eslint-disable-line no-undef +}); + +const p2 = new Promise((res, rej) => { //eslint-disable-line no-unused-vars + consol.log('Two'); // eslint-disable-line no-undef +}); + +const p3 = new Promise((res, rej) => { + consol.log('Three'); // eslint-disable-line no-undef +}); + +new Promise((res, rej) => { + setTimeout(common.mustCall(() => { + p1.catch(() => {}); + p3.catch(() => {}); + })); +}); + +process.on('uncaughtException', (err) => + common.fail('Should not trigger uncaught exception')); + +process.on('exit', () => process._rawDebug('exit event emitted')); diff --git a/test/message/promise_fast_handled_reject.out b/test/message/promise_fast_handled_reject.out new file mode 100644 index 00000000000000..d5970ed4e1a661 --- /dev/null +++ b/test/message/promise_fast_handled_reject.out @@ -0,0 +1,16 @@ +exit event emitted +*test*message*promise_fast_handled_reject.js:* + consol.log('Two'); // eslint-disable-line no-undef + ^ + +ReferenceError: consol is not defined + at *test*message*promise_fast_handled_reject.js:*:* + at Object. (*test*message*promise_fast_handled_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 Module.runMain (module.js:*:*) + at run (bootstrap_node.js:*:*) + at startup (bootstrap_node.js:*:*) diff --git a/test/message/promise_fast_reject.js b/test/message/promise_fast_reject.js new file mode 100644 index 00000000000000..cafc2a27b353d5 --- /dev/null +++ b/test/message/promise_fast_reject.js @@ -0,0 +1,18 @@ +'use strict'; + +// We should always have the stacktrace of the oldest rejection. + +const common = require('../common'); + +new Promise(function(res, rej) { + consol.log('One'); // eslint-disable-line no-undef +}); + +new Promise(function(res, rej) { + consol.log('Two'); // eslint-disable-line no-undef +}); + +process.on('uncaughtException', (err) => + common.fail('Should not trigger uncaught exception')); + +process.on('exit', () => process._rawDebug('exit event emitted')); diff --git a/test/message/promise_fast_reject.out b/test/message/promise_fast_reject.out new file mode 100644 index 00000000000000..ea434d914e4ecd --- /dev/null +++ b/test/message/promise_fast_reject.out @@ -0,0 +1,16 @@ +exit event emitted +*test*message*promise_fast_reject.js:* + consol.log('One'); // eslint-disable-line no-undef + ^ + +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 Module.runMain (module.js:*:*) + at run (bootstrap_node.js:*:*) + at startup (bootstrap_node.js:*:*) diff --git a/test/message/promise_reject.js b/test/message/promise_reject.js new file mode 100644 index 00000000000000..c4e60cfd3bf33a --- /dev/null +++ b/test/message/promise_reject.js @@ -0,0 +1,27 @@ +'use strict'; +const common = require('../common'); + +// Flags: --expose-gc + +Promise.reject(new Error('oops')); + +// Manually call GC due to possible memory contraints with attempting to +// trigger it "naturally". +setTimeout(common.mustCall(() => { + /* eslint-disable no-undef */ + gc(); + gc(); + gc(); + /* eslint-enable no-undef */ +}, 1), 2); + +process.on('beforeExit', () => + common.fail('beforeExit should not be reached')); + +process.on('uncaughtException', (err) => { + // XXX(Fishrock123): This test is currently broken... + console.log(err.stack); + common.fail('Should not trigger uncaught exception'); +}); + +process.on('exit', () => process._rawDebug('exit event emitted')); diff --git a/test/message/promise_reject.out b/test/message/promise_reject.out new file mode 100644 index 00000000000000..af65d739e6ea46 --- /dev/null +++ b/test/message/promise_reject.out @@ -0,0 +1,16 @@ +exit event emitted +*test*message*promise_reject.js:* +Promise.reject(new Error('oops')); + ^ + +Error: oops + at *test*message*promise_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 Module.runMain (module.js:*:*) + at run (bootstrap_node.js:*:*) + at startup (bootstrap_node.js:*:*) + at bootstrap_node.js:*:* diff --git a/test/parallel/test-promises-gc-before-handled.js b/test/parallel/test-promises-gc-before-handled.js new file mode 100644 index 00000000000000..2bf8ac2e96d61f --- /dev/null +++ b/test/parallel/test-promises-gc-before-handled.js @@ -0,0 +1,28 @@ +'use strict'; +const common = require('../common'); + +// Flags: --expose-gc + +var p = new Promise((res, rej) => { + consol.log('oops'); // eslint-disable-line no-undef +}); + +// Manually call GC due to possible memory contraints with attempting to +// trigger it "naturally". +setTimeout(common.mustCall(() => { + /* eslint-disable no-undef */ + gc(); + gc(); + gc(); + /* eslint-enable no-undef */ + setTimeout(common.mustCall(() => { + /* eslint-disable no-undef */ + gc(); + gc(); + gc(); + /* eslint-enable no-undef */ + setTimeout(common.mustCall(() => { + p.catch(() => {}); + }, 1), 250); + }), 20); +}), 20); diff --git a/test/parallel/test-promises-handled-reject.js b/test/parallel/test-promises-handled-reject.js new file mode 100644 index 00000000000000..5b040e1f8b6491 --- /dev/null +++ b/test/parallel/test-promises-handled-reject.js @@ -0,0 +1,19 @@ +'use strict'; +const common = require('../common'); + +// Flags: --expose-gc + +var p = new Promise((res, rej) => { + consol.log('oops'); // eslint-disable-line no-undef +}); + +// Manually call GC due to possible memory contraints with attempting to +// trigger it "naturally". +setTimeout(common.mustCall(() => { + p.catch(() => {}); + /* eslint-disable no-undef */ + gc(); + gc(); + gc(); + /* eslint-enable no-undef */ +}, 1), 2); From 7bc636930a2970d690be466cf4d0c4e1e174f71e Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 10 Feb 2017 11:25:21 -0500 Subject: [PATCH 2/7] src: use std::map for the promise reject map --- src/env.h | 10 +++++++++- src/node.cc | 29 +++++++++++++++-------------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/env.h b/src/env.h index f7482d386b281f..c90b2fd098caea 100644 --- a/src/env.h +++ b/src/env.h @@ -40,6 +40,7 @@ #include #include #include +#include // Caveat emptor: we're going slightly crazy with macros here but the end // hopefully justifies the means. We have a lot of per-context properties @@ -267,7 +268,6 @@ namespace node { V(process_object, v8::Object) \ V(promise_unhandled_rejection_function, v8::Function) \ V(promise_unhandled_rejection, v8::Function) \ - V(promise_unhandled_reject_map, v8::NativeWeakMap) \ V(promise_unhandled_reject_keys, v8::Set) \ V(push_values_to_array_function, v8::Function) \ V(script_context_constructor_template, v8::FunctionTemplate) \ @@ -579,6 +579,14 @@ class Environment { void AddPromiseHook(promise_hook_func fn, void* arg); + struct v8LocalCompare { + bool operator() (const v8::Local& lhs, const v8::Local& rhs) const { + return !lhs->StrictEquals(rhs); + } + }; + + std::map, v8::Local, v8LocalCompare> promise_unhandled_reject_map; + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); diff --git a/src/node.cc b/src/node.cc index ffd2e9b032af4a..710dfd549fa9ba 100644 --- a/src/node.cc +++ b/src/node.cc @@ -85,6 +85,7 @@ #include #include +#include #if defined(NODE_HAVE_I18N_SUPPORT) #include @@ -1294,11 +1295,9 @@ void TrackPromise(const FunctionCallbackInfo& args) { CHECK(args[0]->IsObject()); Local promise = args[0].As(); - TrackPromise::New(env->isolate(), promise); - Local promise_value = GetPromiseReason(env, promise); - Local unhandled_reject_map = - env->promise_unhandled_reject_map(); + std::map, Local, Environment::v8LocalCompare> unhandled_reject_map = + env->promise_unhandled_reject_map; Local unhandled_reject_keys = env->promise_unhandled_reject_keys(); @@ -1306,9 +1305,9 @@ void TrackPromise(const FunctionCallbackInfo& args) { return; } - if (!unhandled_reject_map->Has(promise_value) && + if (unhandled_reject_map.find(promise_value) != unhandled_reject_map.end() && !promise_value->IsUndefined()) { - unhandled_reject_map->Set(promise_value, promise); + unhandled_reject_map.insert(std::make_pair(promise_value, promise)); CHECK(!unhandled_reject_keys->Add(env->context(), promise_value).IsEmpty()); } } @@ -1320,14 +1319,14 @@ void UntrackPromise(const FunctionCallbackInfo& args) { Local promise = args[0].As(); Local err = GetPromiseReason(env, promise); - Local unhandled_reject_map = - env->promise_unhandled_reject_map(); + std::map, Local, Environment::v8LocalCompare> unhandled_reject_map = + env->promise_unhandled_reject_map; Local unhandled_reject_keys = env->promise_unhandled_reject_keys(); if (unhandled_reject_keys->Has(env->context(), err).IsJust()) { CHECK(unhandled_reject_keys->Delete(env->context(), err).IsJust()); - unhandled_reject_map->Delete(err); + unhandled_reject_map.erase(err); } } @@ -1335,7 +1334,8 @@ void SetupPromises(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); - env->set_promise_unhandled_reject_map(NativeWeakMap::New(isolate)); + std::map, Local, Environment::v8LocalCompare> promise_reject_map; + env->promise_unhandled_reject_map = promise_reject_map; env->set_promise_unhandled_reject_keys(Set::New(isolate)); CHECK(args[0]->IsFunction()); @@ -4668,15 +4668,16 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, Null(env.isolate()), 1, &promise_keys_set).ToLocalChecked(); Local promise_keys = ret.As(); uint32_t key_count = promise_keys->Length(); - Local unhandled_reject_map = - env.promise_unhandled_reject_map(); + std::map, Local, Environment::v8LocalCompare> unhandled_reject_map = + env.promise_unhandled_reject_map; for (uint32_t key_iter = 0; key_iter < key_count; key_iter++) { Local key = promise_keys->Get(env.context(), key_iter).ToLocalChecked(); - if (unhandled_reject_map->Has(key)) { - Local promise = unhandled_reject_map->Get(key); + if (unhandled_reject_map.find(key) != unhandled_reject_map.end()) { + Local promise = + unhandled_reject_map.find(key)->second; Local err = GetPromiseReason(&env, promise); Local message = Exception::CreateMessage(isolate, err); From 96967cdb4af21350c886e69086d0ddaf4cc47378 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Tue, 21 Feb 2017 14:30:58 -0500 Subject: [PATCH 3/7] wip move to unordered_map does not work compiles segfaults --- src/env.h | 22 +++++++++++++++++----- src/node.cc | 33 ++++++++++++++++++--------------- src/track-promise.h | 4 ++-- 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/env.h b/src/env.h index c90b2fd098caea..73deda419f5ab2 100644 --- a/src/env.h +++ b/src/env.h @@ -31,6 +31,7 @@ #endif #include "handle_wrap.h" #include "req-wrap.h" +#include "track-promise.h" #include "tree.h" #include "util.h" #include "uv.h" @@ -40,7 +41,7 @@ #include #include #include -#include +#include // Caveat emptor: we're going slightly crazy with macros here but the end // hopefully justifies the means. We have a lot of per-context properties @@ -580,12 +581,23 @@ class Environment { void AddPromiseHook(promise_hook_func fn, void* arg); struct v8LocalCompare { - bool operator() (const v8::Local& lhs, const v8::Local& rhs) const { - return !lhs->StrictEquals(rhs); - } + bool operator() (const v8::Local& lhs, const v8::Local& rhs) const { + return !lhs->StrictEquals(rhs); + } + }; + + struct v8LocalHash { + size_t operator() (const v8::Local& key) const { + if (!key.IsObject()) { + + } + return (size_t) key.As()->GetIdentityHash(); + } }; - std::map, v8::Local, v8LocalCompare> promise_unhandled_reject_map; + std::unordered_map, TrackPromise*, v8LocalHash, v8LocalCompare> promise_unhandled_reject_map; + + // std::map, v8::Local, v8LocalCompare> promise_unhandled_reject_map; private: inline void ThrowError(v8::Local (*fun)(v8::Local), diff --git a/src/node.cc b/src/node.cc index 710dfd549fa9ba..618561ba3924cc 100644 --- a/src/node.cc +++ b/src/node.cc @@ -85,7 +85,7 @@ #include #include -#include +#include #if defined(NODE_HAVE_I18N_SUPPORT) #include @@ -1295,9 +1295,11 @@ void TrackPromise(const FunctionCallbackInfo& args) { CHECK(args[0]->IsObject()); Local promise = args[0].As(); + class TrackPromise* tp = TrackPromise::New(env->isolate(), promise); + Local promise_value = GetPromiseReason(env, promise); - std::map, Local, Environment::v8LocalCompare> unhandled_reject_map = - env->promise_unhandled_reject_map; + std::unordered_map, class TrackPromise*, Environment::v8LocalHash, Environment::v8LocalCompare>* unhandled_reject_map = + &env->promise_unhandled_reject_map; Local unhandled_reject_keys = env->promise_unhandled_reject_keys(); @@ -1305,9 +1307,9 @@ void TrackPromise(const FunctionCallbackInfo& args) { return; } - if (unhandled_reject_map.find(promise_value) != unhandled_reject_map.end() && + if (unhandled_reject_map->find(promise_value) != unhandled_reject_map->end() && !promise_value->IsUndefined()) { - unhandled_reject_map.insert(std::make_pair(promise_value, promise)); + unhandled_reject_map->insert(std::make_pair(promise_value, tp)); CHECK(!unhandled_reject_keys->Add(env->context(), promise_value).IsEmpty()); } } @@ -1319,14 +1321,14 @@ void UntrackPromise(const FunctionCallbackInfo& args) { Local promise = args[0].As(); Local err = GetPromiseReason(env, promise); - std::map, Local, Environment::v8LocalCompare> unhandled_reject_map = - env->promise_unhandled_reject_map; + std::unordered_map, class TrackPromise*, Environment::v8LocalHash, Environment::v8LocalCompare>* unhandled_reject_map = + &env->promise_unhandled_reject_map; Local unhandled_reject_keys = env->promise_unhandled_reject_keys(); if (unhandled_reject_keys->Has(env->context(), err).IsJust()) { CHECK(unhandled_reject_keys->Delete(env->context(), err).IsJust()); - unhandled_reject_map.erase(err); + unhandled_reject_map->erase(err); } } @@ -1334,8 +1336,8 @@ void SetupPromises(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); - std::map, Local, Environment::v8LocalCompare> promise_reject_map; - env->promise_unhandled_reject_map = promise_reject_map; + // std::unordered_map, class TrackPromise, Environment::v8LocalHash, Environment::v8LocalCompare> promise_reject_map; + // env->promise_unhandled_reject_map = &promise_reject_map; env->set_promise_unhandled_reject_keys(Set::New(isolate)); CHECK(args[0]->IsFunction()); @@ -4668,16 +4670,17 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, Null(env.isolate()), 1, &promise_keys_set).ToLocalChecked(); Local promise_keys = ret.As(); uint32_t key_count = promise_keys->Length(); - std::map, Local, Environment::v8LocalCompare> unhandled_reject_map = - env.promise_unhandled_reject_map; + std::unordered_map, class TrackPromise*, Environment::v8LocalHash, Environment::v8LocalCompare>* unhandled_reject_map = + &env.promise_unhandled_reject_map; for (uint32_t key_iter = 0; key_iter < key_count; key_iter++) { Local key = promise_keys->Get(env.context(), key_iter).ToLocalChecked(); - if (unhandled_reject_map.find(key) != unhandled_reject_map.end()) { - Local promise = - unhandled_reject_map.find(key)->second; + if (unhandled_reject_map->find(key) != unhandled_reject_map->end()) { + class TrackPromise* tp = + unhandled_reject_map->find(key)->second; + Local promise = tp->persistent()->Get(isolate); Local err = GetPromiseReason(&env, promise); Local message = Exception::CreateMessage(isolate, err); diff --git a/src/track-promise.h b/src/track-promise.h index 43f7b006eddaad..e7d2bf6a83dfe3 100644 --- a/src/track-promise.h +++ b/src/track-promise.h @@ -13,9 +13,9 @@ class TrackPromise { virtual ~TrackPromise(); static TrackPromise* New(v8::Isolate* isolate, - v8::Local object); + v8::Local object); - inline v8::Persistent* persistent(); + v8::Persistent* persistent(); static inline void WeakCallback( const v8::WeakCallbackInfo& data); From b4b2441acf002c138868de5e5dc568b34525e905 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 28 Feb 2017 23:15:26 +0100 Subject: [PATCH 4/7] WIP --- lib/internal/process/promises.js | 2 + src/env-inl.h | 3 +- src/env.h | 20 +-------- src/node.cc | 67 ++++++----------------------- src/track-promise.cc | 73 +++++++++++++++++++------------- src/track-promise.h | 43 ++++++++++++++----- 6 files changed, 94 insertions(+), 114 deletions(-) diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index f8399cfb3e3daa..7c8507cb2db2a2 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -20,6 +20,8 @@ function setupPromises(scheduleMicrotasks) { return data[data.indexOf('[[PromiseValue]]') + 1]; }, promiseInternals); + Object.assign(process, promiseInternals); // XXX(addaleax) debugging + function unhandledRejection(promise, reason) { hasBeenNotifiedProperty.set(promise, false); addPendingUnhandledRejection(promise, reason); diff --git a/src/env-inl.h b/src/env-inl.h index d4e70ca0d35c9a..b3e5ecdb59e332 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -185,7 +185,8 @@ inline Environment* Environment::GetCurrent( inline Environment::Environment(IsolateData* isolate_data, v8::Local context) - : isolate_(context->GetIsolate()), + : promise_tracker_(this), + isolate_(context->GetIsolate()), isolate_data_(isolate_data), timer_base_(uv_now(isolate_data->event_loop())), using_domains_(false), diff --git a/src/env.h b/src/env.h index 73deda419f5ab2..604c336775caaf 100644 --- a/src/env.h +++ b/src/env.h @@ -269,7 +269,6 @@ namespace node { V(process_object, v8::Object) \ V(promise_unhandled_rejection_function, v8::Function) \ V(promise_unhandled_rejection, v8::Function) \ - V(promise_unhandled_reject_keys, v8::Set) \ V(push_values_to_array_function, v8::Function) \ V(script_context_constructor_template, v8::FunctionTemplate) \ V(script_data_constructor_function, v8::Function) \ @@ -580,24 +579,7 @@ class Environment { void AddPromiseHook(promise_hook_func fn, void* arg); - struct v8LocalCompare { - bool operator() (const v8::Local& lhs, const v8::Local& rhs) const { - return !lhs->StrictEquals(rhs); - } - }; - - struct v8LocalHash { - size_t operator() (const v8::Local& key) const { - if (!key.IsObject()) { - - } - return (size_t) key.As()->GetIdentityHash(); - } - }; - - std::unordered_map, TrackPromise*, v8LocalHash, v8LocalCompare> promise_unhandled_reject_map; - - // std::map, v8::Local, v8LocalCompare> promise_unhandled_reject_map; + PromiseTracker promise_tracker_; private: inline void ThrowError(v8::Local (*fun)(v8::Local), diff --git a/src/node.cc b/src/node.cc index 618561ba3924cc..1193382724504f 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1295,51 +1295,26 @@ void TrackPromise(const FunctionCallbackInfo& args) { CHECK(args[0]->IsObject()); Local promise = args[0].As(); - class TrackPromise* tp = TrackPromise::New(env->isolate(), promise); - - Local promise_value = GetPromiseReason(env, promise); - std::unordered_map, class TrackPromise*, Environment::v8LocalHash, Environment::v8LocalCompare>* unhandled_reject_map = - &env->promise_unhandled_reject_map; - Local unhandled_reject_keys = - env->promise_unhandled_reject_keys(); - - if (unhandled_reject_keys->Size() > 1000) { + if (env->promise_tracker_.Size() > 1000) { return; } - if (unhandled_reject_map->find(promise_value) != unhandled_reject_map->end() && - !promise_value->IsUndefined()) { - unhandled_reject_map->insert(std::make_pair(promise_value, tp)); - CHECK(!unhandled_reject_keys->Add(env->context(), promise_value).IsEmpty()); - } + env->promise_tracker_.TrackPromise(promise); } void UntrackPromise(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK(args[0]->IsObject()); - Local promise = args[0].As(); - - Local err = GetPromiseReason(env, promise); - std::unordered_map, class TrackPromise*, Environment::v8LocalHash, Environment::v8LocalCompare>* unhandled_reject_map = - &env->promise_unhandled_reject_map; - Local unhandled_reject_keys = - env->promise_unhandled_reject_keys(); + Local promise = args[0].As(); - if (unhandled_reject_keys->Has(env->context(), err).IsJust()) { - CHECK(unhandled_reject_keys->Delete(env->context(), err).IsJust()); - unhandled_reject_map->erase(err); - } + env->promise_tracker_.UntrackPromise(promise); } void SetupPromises(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); - // std::unordered_map, class TrackPromise, Environment::v8LocalHash, Environment::v8LocalCompare> promise_reject_map; - // env->promise_unhandled_reject_map = &promise_reject_map; - env->set_promise_unhandled_reject_keys(Set::New(isolate)); - CHECK(args[0]->IsFunction()); CHECK(args[1]->IsFunction()); CHECK(args[2]->IsObject()); @@ -4663,32 +4638,14 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, } while (more == true); } - Local promise_keys_set = - env.promise_unhandled_reject_keys().As(); - Local convert = env.array_from(); - Local ret = convert->Call(env.context(), - Null(env.isolate()), 1, &promise_keys_set).ToLocalChecked(); - Local promise_keys = ret.As(); - uint32_t key_count = promise_keys->Length(); - std::unordered_map, class TrackPromise*, Environment::v8LocalHash, Environment::v8LocalCompare>* unhandled_reject_map = - &env.promise_unhandled_reject_map; - - for (uint32_t key_iter = 0; key_iter < key_count; key_iter++) { - Local key = promise_keys->Get(env.context(), - key_iter).ToLocalChecked(); - - if (unhandled_reject_map->find(key) != unhandled_reject_map->end()) { - class TrackPromise* tp = - unhandled_reject_map->find(key)->second; - Local promise = tp->persistent()->Get(isolate); - Local err = GetPromiseReason(&env, promise); - Local message = Exception::CreateMessage(isolate, err); - - // XXX(Fishrock123): Should this just call ReportException and - // set exit_code = 1 instead? - InternalFatalException(isolate, err, message, true); - } - } + env.promise_tracker_.ForEach([](Environment* env, Local promise) { + Local err = GetPromiseReason(env, promise); + Local message = Exception::CreateMessage(env->isolate(), err); + + // XXX(Fishrock123): Should this just call ReportException and + // set exit_code = 1 instead? + InternalFatalException(env->isolate(), err, message, true); + }); env.set_trace_sync_io(false); diff --git a/src/track-promise.cc b/src/track-promise.cc index 896be59c923e26..f7c621199eef2b 100644 --- a/src/track-promise.cc +++ b/src/track-promise.cc @@ -16,48 +16,63 @@ using v8::Value; using v8::WeakCallbackInfo; using v8::WeakCallbackType; -typedef void (*FreeCallback)(Local object, Local fn); - - -TrackPromise* TrackPromise::New(Isolate* isolate, - Local object) { - return new TrackPromise(isolate, object); +size_t v8ObjectHash::operator() (Persistent* handle) const { + Local object = handle->Get(env->isolate()); + return static_cast(object->GetIdentityHash()); } - -Persistent* TrackPromise::persistent() { - return &persistent_; +bool v8ObjectEquals::operator() (Persistent* lhs, + Persistent* rhs) const { + Local lhs_ = lhs->Get(env->isolate()); + Local rhs_ = rhs->Get(env->isolate()); + return lhs_->StrictEquals(rhs_); } - -TrackPromise::TrackPromise(Isolate* isolate, - Local object) - : persistent_(isolate, object) { - persistent_.SetWeak(this, WeakCallback, WeakCallbackType::kFinalizer); - persistent_.MarkIndependent(); +void PromiseTracker::TrackPromise(Local promise) { + Persistent* p = new Persistent(env_->isolate(), promise); + set_.insert(p); + p->SetWeak(p, WeakCallback, WeakCallbackType::kFinalizer); + p->MarkIndependent(); } - -TrackPromise::~TrackPromise() { - persistent_.Reset(); +void PromiseTracker::UntrackPromise(Local promise) { + Persistent p(env_->isolate(), promise); + auto it = set_.find(&p); + CHECK_NE(it, set_.end()); + (*it)->Reset(); + set_.erase(it); + delete *it; + p.Reset(); } - -void TrackPromise::WeakCallback( - const WeakCallbackInfo& data) { - data.GetParameter()->WeakCallback(data.GetIsolate()); +bool PromiseTracker::HasPromise(Local promise) { + Persistent p(env_->isolate(), promise); + bool found = set_.find(&p) != set_.end(); + p.Reset(); + return found; } +void PromiseTracker::WeakCallback( + const v8::WeakCallbackInfo>& data) { + Environment* env = Environment::GetCurrent(data.GetIsolate()); + env->promise_tracker_.WeakCallback(data.GetParameter()); +} -void TrackPromise::WeakCallback(Isolate* isolate) { - Environment* env = Environment::GetCurrent(isolate); +void PromiseTracker::WeakCallback(v8::Persistent* persistent_) { + Local promise = persistent_->Get(env_->isolate()); + CHECK(HasPromise(promise)); + Local err = node::GetPromiseReason(env_, promise); + Local message = Exception::CreateMessage(env_->isolate(), err); - Local promise = persistent_.Get(isolate); - Local err = node::GetPromiseReason(env, promise); - Local message = Exception::CreateMessage(isolate, err); + node::InternalFatalException(env_->isolate(), err, message, true); + UntrackPromise(promise); +} - node::InternalFatalException(isolate, err, message, true); - delete this; +void PromiseTracker::ForEach(Iterator fn) { + for (auto it = set_.begin(); it != set_.end(); ++it) { + Local object = (*it)->Get(env_->isolate()); + fn(env_, object); + } } } // namespace node diff --git a/src/track-promise.h b/src/track-promise.h index e7d2bf6a83dfe3..703f8b1b8a0e51 100644 --- a/src/track-promise.h +++ b/src/track-promise.h @@ -3,27 +3,50 @@ #include "v8.h" +#include +#include + namespace node { class Environment; -class TrackPromise { +struct v8ObjectHash { + size_t operator() (v8::Persistent* handle) const; + + Environment* env; +}; + +struct v8ObjectEquals { + bool operator() (v8::Persistent* lhs, + v8::Persistent* rhs) const; + + Environment* env; +}; + +class PromiseTracker { public: - TrackPromise(v8::Isolate* isolate, v8::Local object); - virtual ~TrackPromise(); + explicit inline PromiseTracker(Environment* env) + : env_(env), set_(0, v8ObjectHash({env}), v8ObjectEquals({env})) { } + + void TrackPromise(v8::Local promise); + void UntrackPromise(v8::Local promise); + bool HasPromise(v8::Local promise); - static TrackPromise* New(v8::Isolate* isolate, - v8::Local object); + typedef void (*Iterator)(Environment* env, v8::Local promise); - v8::Persistent* persistent(); + void ForEach(Iterator fn); - static inline void WeakCallback( - const v8::WeakCallbackInfo& data); + inline size_t Size() const { return set_.size(); } + static void WeakCallback( + const v8::WeakCallbackInfo>& data); private: - inline void WeakCallback(v8::Isolate* isolate); + inline void WeakCallback(v8::Persistent* promise); - v8::Persistent persistent_; + Environment* env_; + std::unordered_set*, + v8ObjectHash, + v8ObjectEquals> set_; }; } // namespace node From 5d149bd870f581b8de5ea7cd45926ed85437955e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 1 Mar 2017 16:58:30 +0100 Subject: [PATCH 5/7] add promise rejection index so as to always reject the oldest one --- node.gyp | 1 + src/env.h | 2 ++ src/node.cc | 31 ++++++++++++++++++++++++++----- src/track-promise-inl.h | 23 +++++++++++++++++++++++ src/track-promise.cc | 16 +++++----------- src/track-promise.h | 9 ++++++--- 6 files changed, 63 insertions(+), 19 deletions(-) create mode 100644 src/track-promise-inl.h diff --git a/node.gyp b/node.gyp index c8a15b8f1ca4f5..328f180a4d2557 100644 --- a/node.gyp +++ b/node.gyp @@ -232,6 +232,7 @@ 'src/node_i18n.h', 'src/pipe_wrap.h', 'src/track-promise.h', + 'src/track-promise-inl.h', 'src/tty_wrap.h', 'src/tcp_wrap.h', 'src/udp_wrap.h', diff --git a/src/env.h b/src/env.h index 604c336775caaf..dd4e2055cf0135 100644 --- a/src/env.h +++ b/src/env.h @@ -194,6 +194,7 @@ namespace node { V(preference_string, "preference") \ V(priority_string, "priority") \ V(produce_cached_data_string, "produceCachedData") \ + V(promise_rejection_index_string, "_promiseRejectionIndex") \ V(raw_string, "raw") \ V(read_host_object_string, "_readHostObject") \ V(readable_string, "readable") \ @@ -580,6 +581,7 @@ class Environment { void AddPromiseHook(promise_hook_func fn, void* arg); PromiseTracker promise_tracker_; + int64_t promise_tracker_index_ = 0; private: inline void ThrowError(v8::Local (*fun)(v8::Local), diff --git a/src/node.cc b/src/node.cc index 1193382724504f..73452f8d9da254 100644 --- a/src/node.cc +++ b/src/node.cc @@ -60,6 +60,7 @@ #include "string_bytes.h" #include "tracing/agent.h" #include "track-promise.h" +#include "track-promise-inl.h" #include "util.h" #include "uv.h" #if NODE_USE_V8_PLATFORM @@ -1295,6 +1296,11 @@ void TrackPromise(const FunctionCallbackInfo& args) { CHECK(args[0]->IsObject()); Local promise = args[0].As(); + promise->Set(env->context(), + env->promise_rejection_index_string(), + Number::New(env->isolate(), env->promise_tracker_index_++)) + .FromJust(); + if (env->promise_tracker_.Size() > 1000) { return; } @@ -4638,14 +4644,29 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, } while (more == true); } - env.promise_tracker_.ForEach([](Environment* env, Local promise) { - Local err = GetPromiseReason(env, promise); - Local message = Exception::CreateMessage(env->isolate(), err); + Local oldest_rejected_promise; + int64_t lowest_rejection_index = -1; + + env.promise_tracker_.ForEach([&](Environment* env, Local promise) { + Local index_ = + promise->Get(env->context(), env->promise_rejection_index_string()) + .ToLocalChecked(); + CHECK(index_->IsNumber()); + int64_t index = index_->IntegerValue(env->context()).FromJust(); + if (index < lowest_rejection_index || lowest_rejection_index == -1) { + oldest_rejected_promise = promise; + lowest_rejection_index = index; + } + }); + + if (!oldest_rejected_promise.IsEmpty()) { + Local err = GetPromiseReason(&env, oldest_rejected_promise); + Local message = Exception::CreateMessage(isolate, err); // XXX(Fishrock123): Should this just call ReportException and // set exit_code = 1 instead? - InternalFatalException(env->isolate(), err, message, true); - }); + InternalFatalException(isolate, err, message, true); + } env.set_trace_sync_io(false); diff --git a/src/track-promise-inl.h b/src/track-promise-inl.h new file mode 100644 index 00000000000000..073dbafd8903da --- /dev/null +++ b/src/track-promise-inl.h @@ -0,0 +1,23 @@ +#ifndef SRC_TRACK_PROMISE_INL_H_ +#define SRC_TRACK_PROMISE_INL_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "track-promise.h" +#include "env.h" + +namespace node { + +template +void PromiseTracker::ForEach(Iterator fn) { + for (auto it = set_.begin(); it != set_.end(); ++it) { + v8::Local object = (*it)->Get(env_->isolate()); + fn(env_, object); + } +} + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_TRACK_PROMISE_INL_H_ diff --git a/src/track-promise.cc b/src/track-promise.cc index f7c621199eef2b..4e0eedd8638c26 100644 --- a/src/track-promise.cc +++ b/src/track-promise.cc @@ -38,10 +38,11 @@ void PromiseTracker::TrackPromise(Local promise) { void PromiseTracker::UntrackPromise(Local promise) { Persistent p(env_->isolate(), promise); auto it = set_.find(&p); - CHECK_NE(it, set_.end()); - (*it)->Reset(); - set_.erase(it); - delete *it; + if (it != set_.end()) { + (*it)->Reset(); + set_.erase(it); + delete *it; + } p.Reset(); } @@ -68,11 +69,4 @@ void PromiseTracker::WeakCallback(v8::Persistent* persistent_) { UntrackPromise(promise); } -void PromiseTracker::ForEach(Iterator fn) { - for (auto it = set_.begin(); it != set_.end(); ++it) { - Local object = (*it)->Get(env_->isolate()); - fn(env_, object); - } -} - } // namespace node diff --git a/src/track-promise.h b/src/track-promise.h index 703f8b1b8a0e51..605b11740b9be4 100644 --- a/src/track-promise.h +++ b/src/track-promise.h @@ -1,6 +1,8 @@ #ifndef SRC_TRACK_PROMISE_H_ #define SRC_TRACK_PROMISE_H_ +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + #include "v8.h" #include @@ -32,9 +34,8 @@ class PromiseTracker { void UntrackPromise(v8::Local promise); bool HasPromise(v8::Local promise); - typedef void (*Iterator)(Environment* env, v8::Local promise); - - void ForEach(Iterator fn); + template + inline void ForEach(Iterator fn); inline size_t Size() const { return set_.size(); } @@ -51,4 +52,6 @@ class PromiseTracker { } // namespace node +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + #endif // SRC_TRACK_PROMISE_H_ From f43c397451c1fce0e6edff0fc043c78b09a26ca4 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Thu, 23 Mar 2017 17:14:25 -0400 Subject: [PATCH 6/7] [Squash] fix lint, remove debug, cleanup --- lib/internal/process/promises.js | 2 -- src/node.cc | 5 +++-- src/track-promise.cc | 1 - src/track-promise.h | 2 +- test/message/promise_fast_handled_reject.js | 4 ++-- test/message/promise_reject.js | 6 +++--- test/parallel/test-promises-gc-before-handled.js | 2 +- test/parallel/test-promises-handled-reject.js | 2 +- 8 files changed, 11 insertions(+), 13 deletions(-) diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 7c8507cb2db2a2..f8399cfb3e3daa 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -20,8 +20,6 @@ function setupPromises(scheduleMicrotasks) { return data[data.indexOf('[[PromiseValue]]') + 1]; }, promiseInternals); - Object.assign(process, promiseInternals); // XXX(addaleax) debugging - function unhandledRejection(promise, reason) { hasBeenNotifiedProperty.set(promise, false); addPendingUnhandledRejection(promise, reason); diff --git a/src/node.cc b/src/node.cc index 73452f8d9da254..01e46cb90c7153 100644 --- a/src/node.cc +++ b/src/node.cc @@ -142,7 +142,6 @@ using v8::MaybeLocal; using v8::Message; using v8::Name; using v8::NamedPropertyHandlerConfiguration; -using v8::NativeWeakMap; using v8::Null; using v8::Number; using v8::Object; @@ -1301,7 +1300,9 @@ void TrackPromise(const FunctionCallbackInfo& args) { Number::New(env->isolate(), env->promise_tracker_index_++)) .FromJust(); - if (env->promise_tracker_.Size() > 1000) { + // Make some sort of list size check so as to not leak memory. + if (env->promise_tracker_.Size() > 10000) { + // XXX(Fishrock123): Do some intelligent logic here? return; } diff --git a/src/track-promise.cc b/src/track-promise.cc index 4e0eedd8638c26..1e1b5cc17928c5 100644 --- a/src/track-promise.cc +++ b/src/track-promise.cc @@ -6,7 +6,6 @@ namespace node { using v8::Exception; -using v8::Function; using v8::Isolate; using v8::Local; using v8::Message; diff --git a/src/track-promise.h b/src/track-promise.h index 605b11740b9be4..b2e51d52c53bc8 100644 --- a/src/track-promise.h +++ b/src/track-promise.h @@ -14,7 +14,7 @@ class Environment; struct v8ObjectHash { size_t operator() (v8::Persistent* handle) const; - + Environment* env; }; diff --git a/test/message/promise_fast_handled_reject.js b/test/message/promise_fast_handled_reject.js index 3925e63ae7c74e..09c316d1201423 100644 --- a/test/message/promise_fast_handled_reject.js +++ b/test/message/promise_fast_handled_reject.js @@ -5,7 +5,7 @@ const p1 = new Promise((res, rej) => { consol.log('One'); // eslint-disable-line no-undef }); -const p2 = new Promise((res, rej) => { //eslint-disable-line no-unused-vars +const p2 = new Promise((res, rej) => { // eslint-disable-line no-unused-vars consol.log('Two'); // eslint-disable-line no-undef }); @@ -17,7 +17,7 @@ new Promise((res, rej) => { setTimeout(common.mustCall(() => { p1.catch(() => {}); p3.catch(() => {}); - })); + }), 1); }); process.on('uncaughtException', (err) => diff --git a/test/message/promise_reject.js b/test/message/promise_reject.js index c4e60cfd3bf33a..67d80e60579dbe 100644 --- a/test/message/promise_reject.js +++ b/test/message/promise_reject.js @@ -19,9 +19,9 @@ process.on('beforeExit', () => common.fail('beforeExit should not be reached')); process.on('uncaughtException', (err) => { - // XXX(Fishrock123): This test is currently broken... - console.log(err.stack); - common.fail('Should not trigger uncaught exception'); + // XXX(Fishrock123): This test is currently broken... + console.log(err.stack); + common.fail('Should not trigger uncaught exception'); }); process.on('exit', () => process._rawDebug('exit event emitted')); diff --git a/test/parallel/test-promises-gc-before-handled.js b/test/parallel/test-promises-gc-before-handled.js index 2bf8ac2e96d61f..1db9c729294807 100644 --- a/test/parallel/test-promises-gc-before-handled.js +++ b/test/parallel/test-promises-gc-before-handled.js @@ -3,7 +3,7 @@ const common = require('../common'); // Flags: --expose-gc -var p = new Promise((res, rej) => { +const p = new Promise((res, rej) => { consol.log('oops'); // eslint-disable-line no-undef }); diff --git a/test/parallel/test-promises-handled-reject.js b/test/parallel/test-promises-handled-reject.js index 5b040e1f8b6491..8259864e8de8b4 100644 --- a/test/parallel/test-promises-handled-reject.js +++ b/test/parallel/test-promises-handled-reject.js @@ -3,7 +3,7 @@ const common = require('../common'); // Flags: --expose-gc -var p = new Promise((res, rej) => { +const p = new Promise((res, rej) => { consol.log('oops'); // eslint-disable-line no-undef }); From 2ed6fccc8667104cc77f501882049aa3fa730c46 Mon Sep 17 00:00:00 2001 From: Jeremiah Senkpiel Date: Fri, 28 Apr 2017 14:47:38 -0400 Subject: [PATCH 7/7] [Squash] fix rebase issues Tests fail. --- node.gyp | 1 + src/node.cc | 44 +++++++++++++++++++++++++------------------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/node.gyp b/node.gyp index 328f180a4d2557..dd092f3d50e8f1 100644 --- a/node.gyp +++ b/node.gyp @@ -626,6 +626,7 @@ '<(OBJ_TRACING_PATH)/node_trace_buffer.<(OBJ_SUFFIX)', '<(OBJ_TRACING_PATH)/node_trace_writer.<(OBJ_SUFFIX)', '<(OBJ_TRACING_PATH)/trace_event.<(OBJ_SUFFIX)', + '<(OBJ_PATH)/track-promise.<(OBJ_SUFFIX)', ], 'defines': [ diff --git a/src/node.cc b/src/node.cc index 01e46cb90c7153..67684d9fe2dd92 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1259,23 +1259,8 @@ void SetupNextTick(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, fields_count)); } -void PromiseRejectCallback(PromiseRejectMessage message) { - Local promise = message.GetPromise(); - Isolate* isolate = promise->GetIsolate(); - Local value = message.GetValue(); - Local event = Integer::New(isolate, message.GetEvent()); - - Environment* env = Environment::GetCurrent(isolate); - Local callback = env->promise_unhandled_rejection_function(); - - if (value.IsEmpty()) - value = Undefined(isolate); - - Local args[] = { event, promise, value }; - Local process = env->process_object(); +} // anonymous namespace - callback->Call(process, arraysize(args), args); -} Local GetPromiseReason(Environment* env, Local promise) { Local fn = env->promise_unhandled_rejection(); @@ -1289,6 +1274,7 @@ Local GetPromiseReason(Environment* env, Local promise) { &internal_props).ToLocalChecked(); } + void TrackPromise(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1309,6 +1295,7 @@ void TrackPromise(const FunctionCallbackInfo& args) { env->promise_tracker_.TrackPromise(promise); } + void UntrackPromise(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1318,6 +1305,26 @@ void UntrackPromise(const FunctionCallbackInfo& args) { env->promise_tracker_.UntrackPromise(promise); } + +void PromiseRejectCallback(PromiseRejectMessage message) { + Local promise = message.GetPromise(); + Isolate* isolate = promise->GetIsolate(); + Local value = message.GetValue(); + Local event = Integer::New(isolate, message.GetEvent()); + + Environment* env = Environment::GetCurrent(isolate); + Local callback = env->promise_unhandled_rejection_function(); + + if (value.IsEmpty()) + value = Undefined(isolate); + + Local args[] = { event, promise, value }; + Local process = env->process_object(); + + callback->Call(process, arraysize(args), args); +} + + void SetupPromises(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -1338,8 +1345,6 @@ void SetupPromises(const FunctionCallbackInfo& args) { FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupPromises")).FromJust(); } -} // anonymous namespace - void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { Environment* env = Environment::GetCurrent(isolate); @@ -4661,7 +4666,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, }); if (!oldest_rejected_promise.IsEmpty()) { - Local err = GetPromiseReason(&env, oldest_rejected_promise); + Local orp = oldest_rejected_promise.As(); + Local err = GetPromiseReason(&env, orp); Local message = Exception::CreateMessage(isolate, err); // XXX(Fishrock123): Should this just call ReportException and