From 41f333e67949dec26cad3cb8c0bcea686164819b Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Fri, 11 Mar 2016 12:55:59 -0700 Subject: [PATCH 1/5] src,http_parser: remove KickNextTick call Now that HTTPParser uses MakeCallback it is unnecessary to manually process the nextTickQueue. The KickNextTick function is now no longer needed so code has moved back to node::MakeCallback to simplify implementation. Include minor cleanup moving Environment::tick_info() call below the early return to save an operation. PR-URL: https://github.com/nodejs/node/pull/5756 Reviewed-By: Ben Noordhuis Reviewed-By: Andreas Madsen --- src/async-wrap.cc | 7 ++++--- src/env.cc | 23 ----------------------- src/env.h | 2 -- src/node.cc | 18 +++++++++++++++++- src/node_http_parser.cc | 4 ---- 5 files changed, 21 insertions(+), 33 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index e2054031014680..dde07aa0756a3e 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -181,7 +181,6 @@ Local AsyncWrap::MakeCallback(const Local cb, Local post_fn = env()->async_hooks_post_function(); Local uid = Integer::New(env()->isolate(), get_uid()); Local context = object(); - Local process = env()->process_object(); Local domain; bool has_domain = false; @@ -233,16 +232,18 @@ Local AsyncWrap::MakeCallback(const Local cb, } } - Environment::TickInfo* tick_info = env()->tick_info(); - if (callback_scope.in_makecallback()) { return ret; } + Environment::TickInfo* tick_info = env()->tick_info(); + if (tick_info->length() == 0) { env()->isolate()->RunMicrotasks(); } + Local process = env()->process_object(); + if (tick_info->length() == 0) { tick_info->set_index(0); return ret; diff --git a/src/env.cc b/src/env.cc index 75a628face1fe8..ea0ab51cfe922a 100644 --- a/src/env.cc +++ b/src/env.cc @@ -64,27 +64,4 @@ void Environment::PrintSyncTrace() const { fflush(stderr); } - -bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) { - TickInfo* info = tick_info(); - - if (scope->in_makecallback()) { - return true; - } - - if (info->length() == 0) { - isolate()->RunMicrotasks(); - } - - if (info->length() == 0) { - info->set_index(0); - return true; - } - - Local ret = - tick_callback_function()->Call(process_object(), 0, nullptr); - - return !ret.IsEmpty(); -} - } // namespace node diff --git a/src/env.h b/src/env.h index 1e3746b2633a96..0590a8a434b104 100644 --- a/src/env.h +++ b/src/env.h @@ -475,8 +475,6 @@ class Environment { inline int64_t get_async_wrap_uid(); - bool KickNextTick(AsyncCallbackScope* scope); - inline uint32_t* heap_statistics_buffer() const; inline void set_heap_statistics_buffer(uint32_t* pointer); diff --git a/src/node.cc b/src/node.cc index 0d059cc43f3323..ca923585e0fb89 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1222,7 +1222,23 @@ Local MakeCallback(Environment* env, } } - if (!env->KickNextTick(&callback_scope)) { + if (callback_scope.in_makecallback()) { + return ret; + } + + Environment::TickInfo* tick_info = env->tick_info(); + + if (tick_info->length() == 0) { + env->isolate()->RunMicrotasks(); + } + + Local process = env->process_object(); + + if (tick_info->length() == 0) { + tick_info->set_index(0); + } + + if (env->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { return Undefined(env->isolate()); } diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 0b363ca6c310f7..12ae9e1443f534 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -587,8 +587,6 @@ class Parser : public AsyncWrap { if (!cb->IsFunction()) return; - Environment::AsyncCallbackScope callback_scope(parser->env()); - // Hooks for GetCurrentBuffer parser->current_buffer_len_ = nread; parser->current_buffer_data_ = buf->base; @@ -597,8 +595,6 @@ class Parser : public AsyncWrap { parser->current_buffer_len_ = 0; parser->current_buffer_data_ = nullptr; - - parser->env()->KickNextTick(&callback_scope); } From 2dadd8901a85a3e100e8a907b1b64c45dd59eb5a Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 23 Mar 2016 17:02:04 -0600 Subject: [PATCH 2/5] src: reword command and add ternary Make comment clear that Undefined() is returned for legacy compatibility. This will change in the future as a semver-major change, but to be able to port this to previous releases it needs to stay as is. PR-URL: https://github.com/nodejs/node/pull/5756 Reviewed-By: Ben Noordhuis Reviewed-By: Andreas Madsen --- src/node.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/node.cc b/src/node.cc index ca923585e0fb89..bccb4f78a718e3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1205,11 +1205,10 @@ Local MakeCallback(Environment* env, } if (ret.IsEmpty()) { - if (callback_scope.in_makecallback()) - return ret; - // NOTE: Undefined() is returned here for backwards compatibility. - else - return Undefined(env->isolate()); + // NOTE: For backwards compatibility with public API we return Undefined() + // if the top level call threw. + return callback_scope.in_makecallback() ? + ret : Undefined(env->isolate()).As(); } if (has_domain) { From f9938b61418a448a8ef835c1016233e41ed5948e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 22 Feb 2016 16:34:35 -0700 Subject: [PATCH 3/5] async_wrap: setupHooks now accepts object The number of callbacks accepted to setupHooks was getting unwieldy. Instead change the implementation to accept an object with all callbacks PR-URL: https://github.com/nodejs/node/pull/5756 Reviewed-By: Ben Noordhuis Reviewed-By: Andreas Madsen --- src/async-wrap.cc | 35 ++++++++++++++----- .../test-async-wrap-check-providers.js | 2 +- ...st-async-wrap-disabled-propagate-parent.js | 2 +- .../test-async-wrap-propagate-parent.js | 2 +- .../parallel/test-async-wrap-throw-no-init.js | 4 +-- test/parallel/test-async-wrap-uid.js | 2 +- 6 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index dde07aa0756a3e..570ed2f165b2ba 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -121,18 +121,35 @@ static void SetupHooks(const FunctionCallbackInfo& args) { if (env->async_hooks()->callbacks_enabled()) return env->ThrowError("hooks should not be set while also enabled"); - - if (!args[0]->IsFunction()) + if (!args[0]->IsObject()) + return env->ThrowTypeError("first argument must be an object"); + + Local fn_obj = args[0].As(); + + Local init_v = fn_obj->Get( + env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "init")).ToLocalChecked(); + Local pre_v = fn_obj->Get( + env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "pre")).ToLocalChecked(); + Local post_v = fn_obj->Get( + env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "post")).ToLocalChecked(); + Local destroy_v = fn_obj->Get( + env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "destroy")).ToLocalChecked(); + + if (!init_v->IsFunction()) return env->ThrowTypeError("init callback must be a function"); - env->set_async_hooks_init_function(args[0].As()); + env->set_async_hooks_init_function(init_v.As()); - if (args[1]->IsFunction()) - env->set_async_hooks_pre_function(args[1].As()); - if (args[2]->IsFunction()) - env->set_async_hooks_post_function(args[2].As()); - if (args[3]->IsFunction()) - env->set_async_hooks_destroy_function(args[3].As()); + if (pre_v->IsFunction()) + env->set_async_hooks_pre_function(pre_v.As()); + if (post_v->IsFunction()) + env->set_async_hooks_post_function(post_v.As()); + if (destroy_v->IsFunction()) + env->set_async_hooks_destroy_function(destroy_v.As()); } diff --git a/test/parallel/test-async-wrap-check-providers.js b/test/parallel/test-async-wrap-check-providers.js index cd95e7a0f9df57..4b5447b82cbfab 100644 --- a/test/parallel/test-async-wrap-check-providers.js +++ b/test/parallel/test-async-wrap-check-providers.js @@ -36,7 +36,7 @@ function init(id, provider) { function noop() { } -async_wrap.setupHooks(init, noop, noop); +async_wrap.setupHooks({ init }); async_wrap.enable(); diff --git a/test/parallel/test-async-wrap-disabled-propagate-parent.js b/test/parallel/test-async-wrap-disabled-propagate-parent.js index ee674c43ffe6b1..5d5b2b27373690 100644 --- a/test/parallel/test-async-wrap-disabled-propagate-parent.js +++ b/test/parallel/test-async-wrap-disabled-propagate-parent.js @@ -31,7 +31,7 @@ function init(uid, type, parentUid, parentHandle) { function noop() { } -async_wrap.setupHooks(init, noop, noop); +async_wrap.setupHooks({ init }); async_wrap.enable(); server = net.createServer(function(c) { diff --git a/test/parallel/test-async-wrap-propagate-parent.js b/test/parallel/test-async-wrap-propagate-parent.js index c27803832df6fe..ad3fdff0165729 100644 --- a/test/parallel/test-async-wrap-propagate-parent.js +++ b/test/parallel/test-async-wrap-propagate-parent.js @@ -31,7 +31,7 @@ function init(uid, type, parentUid, parentHandle) { function noop() { } -async_wrap.setupHooks(init, noop, noop); +async_wrap.setupHooks({ init }); async_wrap.enable(); server = net.createServer(function(c) { diff --git a/test/parallel/test-async-wrap-throw-no-init.js b/test/parallel/test-async-wrap-throw-no-init.js index 768e38e8eff389..ccf77f66dc7232 100644 --- a/test/parallel/test-async-wrap-throw-no-init.js +++ b/test/parallel/test-async-wrap-throw-no-init.js @@ -7,14 +7,14 @@ const async_wrap = process.binding('async_wrap'); assert.throws(function() { async_wrap.setupHooks(null); -}, /init callback must be a function/); +}, /first argument must be an object/); assert.throws(function() { async_wrap.enable(); }, /init callback is not assigned to a function/); // Should not throw -async_wrap.setupHooks(() => {}); +async_wrap.setupHooks({ init: () => {} }); async_wrap.enable(); assert.throws(function() { diff --git a/test/parallel/test-async-wrap-uid.js b/test/parallel/test-async-wrap-uid.js index 4e664f1bd46ebb..5bf3a8856e0e3f 100644 --- a/test/parallel/test-async-wrap-uid.js +++ b/test/parallel/test-async-wrap-uid.js @@ -6,7 +6,7 @@ const assert = require('assert'); const async_wrap = process.binding('async_wrap'); const storage = new Map(); -async_wrap.setupHooks(init, pre, post, destroy); +async_wrap.setupHooks({ init, pre, post, destroy }); async_wrap.enable(); function init(uid) { From 20337addd6ad134f61684708754be691a3f2ec98 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 22 Feb 2016 22:50:56 -0700 Subject: [PATCH 4/5] async_wrap: notify post if intercepted exception The second argument of the post callback is a boolean indicating whether the callback threw and was intercepted by uncaughtException or a domain. Currently node::MakeCallback has no way of retrieving a uid for the object. This is coming in a future patch. PR-URL: https://github.com/nodejs/node/pull/5756 Reviewed-By: Ben Noordhuis Reviewed-By: Andreas Madsen --- src/async-wrap.cc | 5 ++- src/node.cc | 7 +++- .../test-async-wrap-post-did-throw.js | 34 +++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-async-wrap-post-did-throw.js diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 570ed2f165b2ba..db9d0a4f354015 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -9,6 +9,7 @@ #include "v8-profiler.h" using v8::Array; +using v8::Boolean; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; @@ -231,7 +232,9 @@ Local AsyncWrap::MakeCallback(const Local cb, Local ret = cb->Call(context, argc, argv); if (ran_init_callback() && !post_fn.IsEmpty()) { - if (post_fn->Call(context, 1, &uid).IsEmpty()) + Local did_throw = Boolean::New(env()->isolate(), ret.IsEmpty()); + Local vals[] = { uid, did_throw }; + if (post_fn->Call(context, ARRAY_SIZE(vals), vals).IsEmpty()) FatalError("node::AsyncWrap::MakeCallback", "post hook threw"); } diff --git a/src/node.cc b/src/node.cc index bccb4f78a718e3..83a411050491c6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1200,7 +1200,12 @@ Local MakeCallback(Environment* env, Local ret = callback->Call(recv, argc, argv); if (ran_init_callback && !post_fn.IsEmpty()) { - if (post_fn->Call(object, 0, nullptr).IsEmpty()) + Local did_throw = Boolean::New(env->isolate(), ret.IsEmpty()); + // Currently there's no way to retrieve an uid from node::MakeCallback(). + // This needs to be fixed. + Local vals[] = + { Undefined(env->isolate()).As(), did_throw }; + if (post_fn->Call(object, ARRAY_SIZE(vals), vals).IsEmpty()) FatalError("node::MakeCallback", "post hook threw"); } diff --git a/test/parallel/test-async-wrap-post-did-throw.js b/test/parallel/test-async-wrap-post-did-throw.js new file mode 100644 index 00000000000000..9781983f58987e --- /dev/null +++ b/test/parallel/test-async-wrap-post-did-throw.js @@ -0,0 +1,34 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); +const async_wrap = process.binding('async_wrap'); +var asyncThrows = 0; +var uncaughtExceptionCount = 0; + +process.on('uncaughtException', (e) => { + assert.equal(e.message, 'oh noes!', 'error messages do not match'); +}); + +process.on('exit', () => { + process.removeAllListeners('uncaughtException'); + assert.equal(uncaughtExceptionCount, 1); + assert.equal(uncaughtExceptionCount, asyncThrows); +}); + +function init() { } +function post(id, threw) { + if (threw) + uncaughtExceptionCount++; +} + +async_wrap.setupHooks({ init, post }); +async_wrap.enable(); + +// Timers still aren't supported, so use crypto API. +// It's also important that the callback not happen in a nextTick, like many +// error events in core. +require('crypto').randomBytes(0, () => { + asyncThrows++; + throw new Error('oh noes!'); +}); From a17200b520919a20f72b3353974de82684168b35 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 14 Mar 2016 12:35:22 -0600 Subject: [PATCH 5/5] async_wrap: don't abort on callback exception Rather than abort if the init/pre/post/final/destroy callbacks throw, force the exception to propagate and not be made catchable. This way the application is still not allowed to proceed but also allowed the location of the failure to print before exiting. Though the stack itself may not be of much use since all callbacks except init are called from the bottom of the call stack. /tmp/async-test.js:14 throw new Error('pre'); ^ Error: pre at InternalFieldObject.pre (/tmp/async-test.js:14:9) PR-URL: https://github.com/nodejs/node/pull/5756 Reviewed-By: Ben Noordhuis Reviewed-By: Andreas Madsen --- src/async-wrap-inl.h | 15 ++-- src/async-wrap.cc | 20 ++++-- src/node.cc | 38 +++++++++-- src/node_internals.h | 5 ++ .../test-async-wrap-throw-from-callback.js | 68 +++++++++++++++++++ 5 files changed, 134 insertions(+), 12 deletions(-) create mode 100644 test/parallel/test-async-wrap-throw-from-callback.js diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index e6b24af7fd731f..e61e2ad4bfbf63 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -51,11 +51,15 @@ inline AsyncWrap::AsyncWrap(Environment* env, argv[3] = parent->object(); } + v8::TryCatch try_catch(env->isolate()); + v8::MaybeLocal ret = init_fn->Call(env->context(), object, ARRAY_SIZE(argv), argv); - if (ret.IsEmpty()) - FatalError("node::AsyncWrap::AsyncWrap", "init hook threw"); + if (ret.IsEmpty()) { + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); + } bits_ |= 1; // ran_init_callback() is true now. } @@ -69,10 +73,13 @@ inline AsyncWrap::~AsyncWrap() { if (!fn.IsEmpty()) { v8::HandleScope scope(env()->isolate()); v8::Local uid = v8::Integer::New(env()->isolate(), get_uid()); + v8::TryCatch try_catch(env()->isolate()); v8::MaybeLocal ret = fn->Call(env()->context(), v8::Null(env()->isolate()), 1, &uid); - if (ret.IsEmpty()) - FatalError("node::AsyncWrap::~AsyncWrap", "destroy hook threw"); + if (ret.IsEmpty()) { + ClearFatalExceptionHandlers(env()); + FatalException(env()->isolate(), try_catch); + } } } diff --git a/src/async-wrap.cc b/src/async-wrap.cc index db9d0a4f354015..05ee7fa02ad035 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -18,6 +18,7 @@ using v8::HeapProfiler; using v8::Integer; using v8::Isolate; using v8::Local; +using v8::MaybeLocal; using v8::Object; using v8::RetainedObjectInfo; using v8::TryCatch; @@ -225,8 +226,13 @@ Local AsyncWrap::MakeCallback(const Local cb, } if (ran_init_callback() && !pre_fn.IsEmpty()) { - if (pre_fn->Call(context, 1, &uid).IsEmpty()) - FatalError("node::AsyncWrap::MakeCallback", "pre hook threw"); + TryCatch try_catch(env()->isolate()); + MaybeLocal ar = pre_fn->Call(env()->context(), context, 1, &uid); + if (ar.IsEmpty()) { + ClearFatalExceptionHandlers(env()); + FatalException(env()->isolate(), try_catch); + return Local(); + } } Local ret = cb->Call(context, argc, argv); @@ -234,8 +240,14 @@ Local AsyncWrap::MakeCallback(const Local cb, if (ran_init_callback() && !post_fn.IsEmpty()) { Local did_throw = Boolean::New(env()->isolate(), ret.IsEmpty()); Local vals[] = { uid, did_throw }; - if (post_fn->Call(context, ARRAY_SIZE(vals), vals).IsEmpty()) - FatalError("node::AsyncWrap::MakeCallback", "post hook threw"); + TryCatch try_catch(env()->isolate()); + MaybeLocal ar = + post_fn->Call(env()->context(), context, ARRAY_SIZE(vals), vals); + if (ar.IsEmpty()) { + ClearFatalExceptionHandlers(env()); + FatalException(env()->isolate(), try_catch); + return Local(); + } } if (ret.IsEmpty()) { diff --git a/src/node.cc b/src/node.cc index 83a411050491c6..fa9c3626aa95f3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1193,8 +1193,13 @@ Local MakeCallback(Environment* env, } if (ran_init_callback && !pre_fn.IsEmpty()) { - if (pre_fn->Call(object, 0, nullptr).IsEmpty()) - FatalError("node::MakeCallback", "pre hook threw"); + TryCatch try_catch(env->isolate()); + MaybeLocal ar = pre_fn->Call(env->context(), object, 0, nullptr); + if (ar.IsEmpty()) { + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); + return Local(); + } } Local ret = callback->Call(recv, argc, argv); @@ -1205,8 +1210,14 @@ Local MakeCallback(Environment* env, // This needs to be fixed. Local vals[] = { Undefined(env->isolate()).As(), did_throw }; - if (post_fn->Call(object, ARRAY_SIZE(vals), vals).IsEmpty()) - FatalError("node::MakeCallback", "post hook threw"); + TryCatch try_catch(env->isolate()); + MaybeLocal ar = + post_fn->Call(env->context(), object, ARRAY_SIZE(vals), vals); + if (ar.IsEmpty()) { + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); + return Local(); + } } if (ret.IsEmpty()) { @@ -2404,6 +2415,25 @@ void OnMessage(Local message, Local error) { } +void ClearFatalExceptionHandlers(Environment* env) { + Local process = env->process_object(); + Local events = + process->Get(env->context(), env->events_string()).ToLocalChecked(); + + if (events->IsObject()) { + events.As()->Set( + env->context(), + OneByteString(env->isolate(), "uncaughtException"), + Undefined(env->isolate())).FromJust(); + } + + process->Set( + env->context(), + env->domain_string(), + Undefined(env->isolate())).FromJust(); +} + + static void Binding(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); diff --git a/src/node_internals.h b/src/node_internals.h index 24072e0717dadd..b62c5ff8d50198 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -236,6 +236,11 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator { Environment* env_; }; +// Clear any domain and/or uncaughtException handlers to force the error's +// propagation and shutdown the process. Use this to force the process to exit +// by clearing all callbacks that could handle the error. +void ClearFatalExceptionHandlers(Environment* env); + enum NodeInstanceType { MAIN, WORKER }; class NodeInstanceData { diff --git a/test/parallel/test-async-wrap-throw-from-callback.js b/test/parallel/test-async-wrap-throw-from-callback.js new file mode 100644 index 00000000000000..bfbe32c38b021a --- /dev/null +++ b/test/parallel/test-async-wrap-throw-from-callback.js @@ -0,0 +1,68 @@ +'use strict'; + +require('../common'); +const async_wrap = process.binding('async_wrap'); +const assert = require('assert'); +const crypto = require('crypto'); +const domain = require('domain'); +const spawn = require('child_process').spawn; +const callbacks = [ 'init', 'pre', 'post', 'destroy' ]; +const toCall = process.argv[2]; +var msgCalled = 0; +var msgReceived = 0; + +function init() { + if (toCall === 'init') + throw new Error('init'); +} +function pre() { + if (toCall === 'pre') + throw new Error('pre'); +} +function post() { + if (toCall === 'post') + throw new Error('post'); +} +function destroy() { + if (toCall === 'destroy') + throw new Error('destroy'); +} + +if (typeof process.argv[2] === 'string') { + async_wrap.setupHooks({ init, pre, post, destroy }); + async_wrap.enable(); + + process.on('uncaughtException', () => assert.ok(0, 'UNREACHABLE')); + + const d = domain.create(); + d.on('error', () => assert.ok(0, 'UNREACHABLE')); + d.run(() => { + // Using randomBytes because timers are not yet supported. + crypto.randomBytes(0, () => { }); + }); + +} else { + + process.on('exit', (code) => { + assert.equal(msgCalled, callbacks.length); + assert.equal(msgCalled, msgReceived); + }); + + callbacks.forEach((item) => { + msgCalled++; + + const child = spawn(process.execPath, [__filename, item]); + var errstring = ''; + + child.stderr.on('data', (data) => { + errstring += data.toString(); + }); + + child.on('close', (code) => { + if (errstring.includes('Error: ' + item)) + msgReceived++; + + assert.equal(code, 1, `${item} closed with code ${code}`); + }); + }); +}