From 2cbbaafca9c81a4e46e053405ded863ea47637c4 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 14 Mar 2016 12:35:22 -0600 Subject: [PATCH] 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 1bd995d789fc87..b5bffa113156df 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1175,8 +1175,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); @@ -1187,8 +1192,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()) { @@ -2363,6 +2374,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 0acd99c9fd30f7..0523a3f8bc0a6b 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -234,6 +234,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}`); + }); + }); +}