From ac197de314c59ab57da03cfa57b829ce1402fc16 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 15 Feb 2019 14:33:23 +0100 Subject: [PATCH] src: only call .ReThrow() if not terminating Otherwise, it looks like a `null` exception is being thrown. PR-URL: https://github.com/nodejs/node/pull/26130 Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius --- src/module_wrap.cc | 24 ++++++++++--------- src/node_contextify.cc | 17 +++++++++---- .../es-modules/import-process-exit.mjs | 1 + test/fixtures/es-modules/process-exit.mjs | 2 ++ test/parallel/test-worker-esm-exit.js | 10 ++++++++ 5 files changed, 38 insertions(+), 16 deletions(-) create mode 100644 test/fixtures/es-modules/import-process-exit.mjs create mode 100644 test/fixtures/es-modules/process-exit.mjs create mode 100644 test/parallel/test-worker-esm-exit.js diff --git a/src/module_wrap.cc b/src/module_wrap.cc index f642440bff5a88..93e791b52240aa 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -158,12 +158,13 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { Context::Scope context_scope(context); ScriptCompiler::Source source(source_text, origin); if (!ScriptCompiler::CompileModule(isolate, &source).ToLocal(&module)) { - CHECK(try_catch.HasCaught()); - CHECK(!try_catch.Message().IsEmpty()); - CHECK(!try_catch.Exception().IsEmpty()); - AppendExceptionLine(env, try_catch.Exception(), try_catch.Message(), - ErrorHandlingMode::MODULE_ERROR); - try_catch.ReThrow(); + if (try_catch.HasCaught() && !try_catch.HasTerminated()) { + CHECK(!try_catch.Message().IsEmpty()); + CHECK(!try_catch.Exception().IsEmpty()); + AppendExceptionLine(env, try_catch.Exception(), try_catch.Message(), + ErrorHandlingMode::MODULE_ERROR); + try_catch.ReThrow(); + } return; } } @@ -245,13 +246,12 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo& args) { Local context = obj->context_.Get(isolate); Local module = obj->module_.Get(isolate); TryCatchScope try_catch(env); - Maybe ok = module->InstantiateModule(context, ResolveCallback); + USE(module->InstantiateModule(context, ResolveCallback)); // clear resolve cache on instantiate obj->resolve_cache_.clear(); - if (!ok.FromMaybe(false)) { - CHECK(try_catch.HasCaught()); + if (try_catch.HasCaught() && !try_catch.HasTerminated()) { CHECK(!try_catch.Message().IsEmpty()); CHECK(!try_catch.Exception().IsEmpty()); AppendExceptionLine(env, try_catch.Exception(), try_catch.Message(), @@ -300,6 +300,8 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo& args) { // Convert the termination exception into a regular exception. if (timed_out || received_signal) { + if (!env->is_main_thread() && env->is_stopping_worker()) + return; env->isolate()->CancelTerminateExecution(); // It is possible that execution was terminated by another timeout in // which this timeout is nested, so check whether one of the watchdogs @@ -312,7 +314,8 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo& args) { } if (try_catch.HasCaught()) { - try_catch.ReThrow(); + if (!try_catch.HasTerminated()) + try_catch.ReThrow(); return; } @@ -375,7 +378,6 @@ void ModuleWrap::GetError(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&obj, args.This()); Local module = obj->module_.Get(isolate); - args.GetReturnValue().Set(module->GetException()); } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 61f60576d2f2a4..0ba2b1a90cb93b 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -252,7 +252,8 @@ void ContextifyContext::MakeContext(const FunctionCallbackInfo& args) { ContextifyContext* context = new ContextifyContext(env, sandbox, options); if (try_catch.HasCaught()) { - try_catch.ReThrow(); + if (!try_catch.HasTerminated()) + try_catch.ReThrow(); return; } @@ -729,7 +730,8 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { if (v8_script.IsEmpty()) { DecorateErrorStack(env, try_catch); no_abort_scope.Close(); - try_catch.ReThrow(); + if (!try_catch.HasTerminated()) + try_catch.ReThrow(); TRACE_EVENT_NESTABLE_ASYNC_END0( TRACING_CATEGORY_NODE2(vm, script), "ContextifyScript::New", @@ -922,6 +924,8 @@ bool ContextifyScript::EvalMachine(Environment* env, // Convert the termination exception into a regular exception. if (timed_out || received_signal) { + if (!env->is_main_thread() && env->is_stopping_worker()) + return false; env->isolate()->CancelTerminateExecution(); // It is possible that execution was terminated by another timeout in // which this timeout is nested, so check whether one of the watchdogs @@ -944,7 +948,8 @@ bool ContextifyScript::EvalMachine(Environment* env, // letting try_catch catch it. // If execution has been terminated, but not by one of the watchdogs from // this invocation, this will re-throw a `null` value. - try_catch.ReThrow(); + if (!try_catch.HasTerminated()) + try_catch.ReThrow(); return false; } @@ -1098,8 +1103,10 @@ void ContextifyContext::CompileFunction( context_extensions.size(), context_extensions.data(), options); if (maybe_fn.IsEmpty()) { - DecorateErrorStack(env, try_catch); - try_catch.ReThrow(); + if (try_catch.HasCaught() && !try_catch.HasTerminated()) { + DecorateErrorStack(env, try_catch); + try_catch.ReThrow(); + } return; } Local fn = maybe_fn.ToLocalChecked(); diff --git a/test/fixtures/es-modules/import-process-exit.mjs b/test/fixtures/es-modules/import-process-exit.mjs new file mode 100644 index 00000000000000..aa294f1bcbe6df --- /dev/null +++ b/test/fixtures/es-modules/import-process-exit.mjs @@ -0,0 +1 @@ +import exit from "./process-exit.mjs"; diff --git a/test/fixtures/es-modules/process-exit.mjs b/test/fixtures/es-modules/process-exit.mjs new file mode 100644 index 00000000000000..869ce08efa56cb --- /dev/null +++ b/test/fixtures/es-modules/process-exit.mjs @@ -0,0 +1,2 @@ +process.exit(42); +export default null; diff --git a/test/parallel/test-worker-esm-exit.js b/test/parallel/test-worker-esm-exit.js new file mode 100644 index 00000000000000..c0b9d874895725 --- /dev/null +++ b/test/parallel/test-worker-esm-exit.js @@ -0,0 +1,10 @@ +'use strict'; +const common = require('../common'); +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); + +const w = new Worker(fixtures.path('es-modules/import-process-exit.mjs'), + { execArgv: ['--experimental-modules'] }); +w.on('error', common.mustNotCall()); +w.on('exit', (code) => assert.strictEqual(code, 42));