src: only call .ReThrow() if not terminating

Otherwise, it looks like a `null` exception is being thrown.
nodejs · Feb 19, 2019 · 2ad7f95 · 2ad7f95
1 parent 1c71c0c
commit 2ad7f95
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 16 deletions.
diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -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;
     }
   }
@@ -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(),
@@ -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
@@ -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;
   }
 
@@ -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());
 }
 

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -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;
   }
 
@@ -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",
@@ -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<Function> fn = maybe_fn.ToLocalChecked();

diff --git a/test/fixtures/es-modules/import-process-exit.mjs 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
@@ -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
@@ -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));