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 e2054031014680..05ee7fa02ad035 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; @@ -17,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; @@ -121,18 +123,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()); } @@ -181,7 +200,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; @@ -208,15 +226,28 @@ 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); if (ran_init_callback() && !post_fn.IsEmpty()) { - if (post_fn->Call(context, 1, &uid).IsEmpty()) - FatalError("node::AsyncWrap::MakeCallback", "post hook threw"); + Local did_throw = Boolean::New(env()->isolate(), ret.IsEmpty()); + Local vals[] = { uid, did_throw }; + 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()) { @@ -233,16 +264,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..fa9c3626aa95f3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1193,23 +1193,38 @@ 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); if (ran_init_callback && !post_fn.IsEmpty()) { - if (post_fn->Call(object, 0, nullptr).IsEmpty()) - FatalError("node::MakeCallback", "post hook threw"); + 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 }; + 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()) { - 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) { @@ -1222,7 +1237,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()); } @@ -2384,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_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); } 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-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-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!'); +}); 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-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}`); + }); + }); +} 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) {