Skip to content

Commit

Permalink
src: only call .ReThrow() if not terminating
Browse files Browse the repository at this point in the history
Otherwise, it looks like a `null` exception is being thrown.

PR-URL: #26130
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
  • Loading branch information
addaleax authored and rvagg committed Feb 28, 2019
1 parent 818b280 commit bd40a12
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 16 deletions.
24 changes: 13 additions & 11 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,13 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& 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;
}
}
Expand Down Expand Up @@ -245,13 +246,12 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {
Local<Context> context = obj->context_.Get(isolate);
Local<Module> module = obj->module_.Get(isolate);
TryCatchScope try_catch(env);
Maybe<bool> 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(),
Expand Down Expand Up @@ -300,6 +300,8 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& 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
Expand All @@ -312,7 +314,8 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
}

if (try_catch.HasCaught()) {
try_catch.ReThrow();
if (!try_catch.HasTerminated())
try_catch.ReThrow();
return;
}

Expand Down Expand Up @@ -375,7 +378,6 @@ void ModuleWrap::GetError(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&obj, args.This());

Local<Module> module = obj->module_.Get(isolate);

args.GetReturnValue().Set(module->GetException());
}

Expand Down
17 changes: 12 additions & 5 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,8 @@ void ContextifyContext::MakeContext(const FunctionCallbackInfo<Value>& args) {
ContextifyContext* context = new ContextifyContext(env, sandbox, options);

if (try_catch.HasCaught()) {
try_catch.ReThrow();
if (!try_catch.HasTerminated())
try_catch.ReThrow();
return;
}

Expand Down Expand Up @@ -729,7 +730,8 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& 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",
Expand Down Expand Up @@ -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
Expand All @@ -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;
}
Expand Down Expand Up @@ -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<Function> fn = maybe_fn.ToLocalChecked();
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/es-modules/import-process-exit.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import exit from "./process-exit.mjs";
2 changes: 2 additions & 0 deletions test/fixtures/es-modules/process-exit.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
process.exit(42);
export default null;
10 changes: 10 additions & 0 deletions test/parallel/test-worker-esm-exit.js
Original file line number Diff line number Diff line change
@@ -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));

0 comments on commit bd40a12

Please sign in to comment.