fix(worker): terminate() reaches a worker still inside its entry script (#493)

* fix(worker): terminate() reaches a worker still inside its entry script

The worker's isolate was published to its wrapper only after the startup
function returned, i.e. after the entry script had finished evaluating, so
Terminate() found nothing to interrupt for a worker still in its entry: a
synchronous loop there ran forever, and a parked top-level await ended
only when the loader's settle deadline expired. Node and the web both
interrupt the running entry.

The isolate is now published right after the worker runtime is
initialized, before the entry runs, and the startup function honors a
terminate() that landed earlier by checking the flag before any app code.
A termination that lands inside the entry unwinds RunModule, which no
longer rethrows or reports on a terminating isolate, and the thread goes
straight to its existing teardown path. The module runner and the
exception constructor name a caught termination for what it is instead of
formatting the TryCatch, and the settle pump checks for one after its loop
too, so a termination is never reported as a timeout or as an entry
rejection.

Also documents the real resourceLimits and ios.priority Worker options in
docs/worker-threads.md, whose node:worker_threads table claimed the runtime
imposed no limits.

Suite 1735/0, also under AddressSanitizer.

Fixes #445

* fix(worker): keep a termination from being re-armed as an error on its way out

A termination that interrupts a CommonJS entry unwinds through the native
require callback, whose catch re-threw the message-only failure onto the
isolate as an ordinary Error: V8 then reported an exception rather than a
termination to RunModule's TryCatch, which built a full error report before
the worker boundary dropped it. ReThrowToV8 now leaves a terminating isolate
alone, since the pending termination is the failure, and RunModule consults
the same three termination signals as the settle pump.

The pumped graph walk ahead of an entry's compile bails on termination but
reports nothing, so LoadESModule went on to the synchronous HTTP loader, which
can block on the network. Both walks are now followed by a termination check.

Also refreshes the drain comment that described the pre-publication isolate
window, widens the startup-sweep spec's spread so its rounds reach the settle
pump, and corrects the resourceLimits RangeError wording.

* fix(worker): consult the termination-requested flag at the last two failure sites

The CommonJS module-function call and the ES module Evaluate failure only
recognized a termination V8 had already materialized. A request that landed
during the call, before any JS ran to materialize it, let an ordinary failure
be formatted as an error on a terminating isolate. Both sites now read the
same three signals as the settle pump and the require path.

* fix(worker): rethrow ordinary failures until a termination materializes

ReThrowToV8 skipped the throw whenever the runtime's termination-requested
flag was set. Between terminate() setting that flag and V8 delivering the
interrupt, JS still runs, so an ordinary native failure in that window
returned nothing: require() yielded undefined and a native constructor
handed back an uninitialized object, which a following call could trip an
assert on in Debug builds. The skip now keys on IsExecutionTerminating()
alone; inside a callback a materialized termination is never cleared on the
way up, so that signal is sufficient there.

Also gives the module-instantiate failure in LoadESModule the same
termination check as the runner's other failure sites, so a terminated link
is reported as an interruption rather than logged as an instantiation error.

Author: edusperoni

NativeScript/runtime/DataWrapper.h

@@ -558,12 +558,20 @@ class WorkerWrapper : public BaseDataWrapper {
   void CreateInspector(v8::Isolate* isolate, const std::string& scriptPath);
   void DestroyInspector();
 
+  // `func` runs on the worker thread: it creates the worker's runtime, hands
+  // the isolate to PublishIsolate once the runtime is initialized, and runs
+  // the entry script.
   void Start(std::shared_ptr<v8::Persistent<v8::Value>> poWorker,
-             std::function<v8::Isolate*()> func,
+             std::function<void()> func,
              std::optional<int> qualityOfService = std::nullopt);
+  // Worker thread, once the runtime is initialized and before the entry script
+  // runs. From here Terminate() reaches V8: it interrupts whatever JS the
+  // worker runs, the entry script included, and flags the module pumps so a
+  // parked graph stops waiting. A terminate() that landed earlier is honored by
+  // the caller checking IsTerminating() right after this, before any app code.
+  void PublishIsolate(v8::Isolate* isolate);
   // Both reporters take the isolate from their caller, which is running on
-  // it: they are reachable while the entry script is still evaluating, before
-  // workerIsolate_ is published.
+  // it: they are reachable while the entry script is still evaluating.
   void CallOnErrorHandlers(v8::Isolate* isolate, v8::TryCatch& tc);
   // Reports a rejected entry-evaluation promise. A rejection carries a reason
   // rather than a TryCatch, so it cannot go through CallOnErrorHandlers, but it
@@ -607,11 +615,6 @@ class WorkerWrapper : public BaseDataWrapper {
   // used to name the cap in the message forwarded to the parent.
   void WatchHeapLimit(v8::Isolate* isolate, const std::string& scriptPath,
                       std::optional<size_t> maxOldGenerationSizeBytes);
-  // Whether the heap cap was hit. The worker startup path checks this to stop
-  // before running anything else in an isolate V8 is terminating.
-  inline bool HeapLimitExceeded() const {
-    return heapLimitExceeded_.load(std::memory_order_acquire);
-  }
   // The JS Worker object is a GC root from a successful start until the worker
   // ends, so a running worker is reachable the way a browser's is rather than
   // depending on its finalizer to keep it. Both of these run on the main
@@ -630,6 +633,9 @@ class WorkerWrapper : public BaseDataWrapper {
   const int Id();
   const bool IsRunning();
   const bool IsClosing();
+  // Set by Terminate() from any thread, by the near-heap-limit callback, and by
+  // the worker thread itself once a close() takes effect. One-way.
+  const bool IsTerminating();
   const int WorkerId();
   const inline v8::Isolate* GetMainIsolate() { return mainIsolate_; }
   // The only route from the worker thread to the parent: see mainLoop_.
@@ -658,9 +664,13 @@ class WorkerWrapper : public BaseDataWrapper {
   enum class Holders : uint8_t { Parent, Both, WorkerThread };
 
   v8::Isolate* mainIsolate_;
-  // Written by the worker thread only: published once the worker's startup
-  // function returns, withdrawn before the worker's runtime is deleted. Any
-  // other thread reads and uses it under workerIsolateMutex_.
+  // Written by the worker thread only: published by PublishIsolate once the
+  // worker's runtime is initialized and before its entry script runs,
+  // withdrawn before the runtime is deleted. Any other thread reads and uses
+  // it under workerIsolateMutex_. Null while the runtime is being set up, so a
+  // Terminate() in that window cannot interrupt the builtins Runtime::Init
+  // evaluates; the startup function checks the terminating flag right after
+  // publishing instead.
   v8::Isolate* workerIsolate_;
   std::mutex workerIsolateMutex_;
   std::atomic<bool> isRunning_;
@@ -690,10 +700,11 @@ class WorkerWrapper : public BaseDataWrapper {
   // thread) and DestroyInspector() (worker thread) agree on liveness.
   v8_inspector::WorkerInspectorClient* inspector_ = nullptr;
   std::mutex inspectorMutex_;
-  // The worker isolate as seen from the heap-limit callback. Separate from
-  // workerIsolate_, which BackgroundLooper only publishes once the entry script
-  // has finished evaluating — the point at which a heap cap is most likely to
-  // be hit is inside that entry.
+  // The isolate the near-heap-limit callback is armed on. Worker thread only.
+  // Kept apart from workerIsolate_ because the callback is armed before the
+  // isolate is published and can fire during Runtime::Init, and because
+  // removing a callback V8 never had registered is fatal: non-null here means
+  // exactly "armed, remove at teardown".
   v8::Isolate* heapLimitIsolate_ = nullptr;
   std::string heapLimitMessage_;
   std::string heapLimitSource_;
@@ -715,7 +726,7 @@ class WorkerWrapper : public BaseDataWrapper {
   // task would run on.
   std::shared_ptr<std::atomic<WorkerWrapper*>> selfRef_;
 
-  void BackgroundLooper(std::function<v8::Isolate*()> func);
+  void BackgroundLooper(std::function<void()> func);
   void DrainPendingTasks();
   void ForwardErrorPayloadToMain(const std::string& message,
                                  const std::string& source,

NativeScript/runtime/ModuleInternal.mm

@@ -258,7 +258,10 @@ static bool IsHttpModulePath(const std::string& path) {
       moduleNamespace =
           ModuleInternal::LoadESModule(isolate, path, BootEntryEvaluationOptions(isHttpModule));
     } catch (const NativeScriptException& ex) {
-      if (RuntimeConfig.IsDebug) {
+      Runtime* runtime = Runtime::GetRuntime(isolate);
+      bool terminating = isolate->IsExecutionTerminating() ||
+                         (runtime != nullptr && runtime->IsTerminationRequested());
+      if (RuntimeConfig.IsDebug && !terminating) {
         Log(@"***** JavaScript exception occurred *****");
         Log(@"Error loading ES module: %s", path.c_str());
         Log(@"Exception: %s", ex.getMessage().c_str());
@@ -293,6 +296,15 @@ throw NativeScriptException(
   success = requireFunc->Call(context, globalObject, 1, args).ToLocal(&result);
 
   if (!success || tc.HasCaught()) {
+    // A termination is caught like an exception but carries no error value;
+    // naming it as the failure keeps it from being reported as one. All three
+    // signals, as in the settle pump: a native frame between here and the
+    // interrupted JS may have swallowed the sentinel on its way out.
+    Runtime* runtime = Runtime::GetRuntime(isolate);
+    if (tc.HasTerminated() || isolate->IsExecutionTerminating() ||
+        (runtime != nullptr && runtime->IsTerminationRequested())) {
+      throw NativeScriptException("Module evaluation interrupted by isolate termination: " + path);
+    }
     if (RuntimeConfig.IsDebug) {
       Log(@"***** JavaScript exception occurred *****");
       Log(@"Error in require() call:");
@@ -882,6 +894,12 @@ throw NativeScriptException(isolate,
         moduleFunc->Call(context, thiz, sizeof(requireArgs) / sizeof(Local<Value>), requireArgs)
             .ToLocal(&result);
     if (!success || tc.HasCaught()) {
+      Runtime* runtime = Runtime::GetRuntime(isolate);
+      if (tc.HasTerminated() || isolate->IsExecutionTerminating() ||
+          (runtime != nullptr && runtime->IsTerminationRequested())) {
+        throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                                    modulePath);
+      }
       throw NativeScriptException(isolate, tc, "Error calling module function");
     }
   }
@@ -1277,6 +1295,15 @@ throw NativeScriptException(
   Local<Value> result;
   if (!module->Evaluate(context).ToLocal(&result)) {
     RemoveModuleFromRegistry(isolate, canonicalPath);
+    // Same rule as the pump below: a termination outranks any failure detail,
+    // and reading the TryCatch as an error would run JS on the dying isolate.
+    Runtime* runtime = Runtime::GetRuntime(isolate);
+    if (tcEval.HasTerminated() || isolate->IsExecutionTerminating() ||
+        (runtime != nullptr && runtime->IsTerminationRequested())) {
+      LogEsmPhase(canonicalPath, "evaluate", "terminated");
+      throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                                  canonicalPath);
+    }
     const char* classification = "unknown";
     if (tcEval.HasCaught()) {
       Local<Message> msg = tcEval.Message();
@@ -1364,32 +1391,38 @@ throw NativeScriptException("ES module " + canonicalPath +
   NSDate* deadline = [NSDate dateWithTimeIntervalSinceNow:options.deadlineSeconds];
   bool settled = false;
 
+  // Termination outranks a settled result and a timeout alike. Handing back a
+  // namespace would send the caller on to run more JS — enabling a queue,
+  // draining messages — on an isolate V8 has already been told to stop, and
+  // reporting a timeout would name a reason that is not the real one.
+  //
+  // Three signals are consulted: V8 only reports a termination it has already
+  // materialized, which needs JS to run, and a graph parked on a promise
+  // nothing settles never gives it any; one that materialized inside a
+  // checkpoint lands in promiseTc. Message-only exception: building a V8
+  // error on a terminating isolate is not allowed.
+  auto throwIfTerminating = [&]() {
+    if (!promiseTc.HasTerminated() && !isolate->IsExecutionTerminating() &&
+        (runtime == nullptr || !runtime->IsTerminationRequested())) {
+      return;
+    }
+    LogEsmPhase(canonicalPath, "evaluate", "terminated");
+    // Probed, not consumed: only a still-pending promise leaves a
+    // half-evaluated module in the registry. One that already settled is
+    // complete, and evicting it would throw away a good entry for no reason
+    // — the result simply goes unused.
+    if (promise->State() == Promise::kPending) {
+      RemoveModuleFromRegistry(isolate, canonicalPath);
+    }
+    throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                                canonicalPath);
+  };
+
   // State is checked before the first pump: a synchronous graph's
   // evaluation promise is already settled when Evaluate() returns, so it
   // exits here without paying for a runloop slice.
   while (!promiseTc.HasCaught()) {
-    // Termination outranks a settled result. Handing back a namespace here
-    // would send the caller on to run more JS — enabling a queue, draining
-    // messages — on an isolate V8 has already been told to stop, so a
-    // termination seen at the loop head always throws, settled or not.
-    //
-    // Both signals are consulted: V8 only reports a termination it has already
-    // materialized, which needs JS to run, and a graph parked on a promise
-    // nothing settles never gives it any. Message-only exception: building a
-    // V8 error on a terminating isolate is not allowed.
-    if (isolate->IsExecutionTerminating() ||
-        (runtime != nullptr && runtime->IsTerminationRequested())) {
-      LogEsmPhase(canonicalPath, "evaluate", "terminated");
-      // Probed, not consumed: only a still-pending promise leaves a
-      // half-evaluated module in the registry. One that already settled is
-      // complete, and evicting it would throw away a good entry for no reason
-      // — the result simply goes unused.
-      if (promise->State() == Promise::kPending) {
-        RemoveModuleFromRegistry(isolate, canonicalPath);
-      }
-      throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
-                                  canonicalPath);
-    }
+    throwIfTerminating();
 
     Promise::PromiseState state = promise->State();
     if (state != Promise::kPending) {
@@ -1411,6 +1444,11 @@ throw NativeScriptException("Module evaluation interrupted by isolate terminatio
     }
   }
 
+  // The loop leaves through its condition when a pump materializes the
+  // termination, and through the deadline when the request arrived during the
+  // final slice.
+  throwIfTerminating();
+
   if (!settled && promise->State() == Promise::kPending) {
     LogEsmPhase(canonicalPath, "evaluate", "promise-timeout");
     if (options.timeoutBehavior == ModuleEvaluationOptions::TimeoutBehavior::kThrow) {
@@ -1434,6 +1472,21 @@ throw NativeScriptException("Module evaluation interrupted by isolate terminatio
   return MaybeLocal<Promise>();
 }
 
+// A pumped graph walk bails on a termination request but reports nothing;
+// what follows it would compile, fetch synchronously or evaluate on an
+// isolate that must not run anything more.
+static void ThrowIfLoadInterruptedByTermination(Isolate* isolate,
+                                                const std::string& canonicalPath) {
+  Runtime* runtime = Runtime::GetRuntime(isolate);
+  if (!isolate->IsExecutionTerminating() &&
+      (runtime == nullptr || !runtime->IsTerminationRequested())) {
+    return;
+  }
+  LogEsmPhase(canonicalPath, "load", "terminated");
+  throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                              canonicalPath);
+}
+
 Local<Value> ModuleInternal::LoadESModule(Isolate* isolate, const std::string& path,
                                           const ModuleEvaluationOptions& options) {
   bool isHttpModule = IsHttpModulePath(path);
@@ -1513,6 +1566,7 @@ throw NativeScriptException("Module evaluation interrupted by isolate terminatio
     // registry hit and instantiation resolves as pure lookup. On timeout or
     // partial coverage the legacy synchronous path still owns correctness.
     RunModuleGraphLoadPumped(isolate, context, requestPath, kModuleEvaluateDeadlineSeconds);
+    ThrowIfLoadInterruptedByTermination(isolate, canonicalPath);
     MaybeLocal<Module> maybeMod = LoadHttpModuleForUrl(isolate, context, requestPath);
     if (!maybeMod.ToLocal(&module)) {
       logPhase("compile", "fail", "http-loader");
@@ -1536,6 +1590,7 @@ throw NativeScriptException("Module evaluation interrupted by isolate terminatio
     // async fetch, so it legitimately waits here and then evaluates
     // synchronously.
     RunModuleGraphLoadPumped(isolate, context, canonicalPath, kModuleEvaluateDeadlineSeconds);
+    ThrowIfLoadInterruptedByTermination(isolate, canonicalPath);
     auto walkedIt = registry.find(canonicalPath);
     if (walkedIt != registry.end()) {
       Local<Module> walked = walkedIt->second.Get(isolate);
@@ -1597,6 +1652,13 @@ throw NativeScriptException(isolate, tcCompile,
 
     if (!linked) {
       RemoveModuleFromRegistry(isolate, canonicalPath);
+      Runtime* runtime = Runtime::GetRuntime(isolate);
+      if (tcLink.HasTerminated() || isolate->IsExecutionTerminating() ||
+          (runtime != nullptr && runtime->IsTerminationRequested())) {
+        logPhase("instantiate", "terminated");
+        throw NativeScriptException("Module evaluation interrupted by isolate termination: " +
+                                    canonicalPath);
+      }
       const char* classification = "unknown";
       if (tcLink.HasCaught()) {
         Local<Message> msg = tcLink.Message();

NativeScript/runtime/NativeScriptException.mm

@@ -85,6 +85,15 @@ static void ConsiderStackCandidate(PendingErrorDisplay& state, v8::Isolate* isol
 
 NativeScriptException::NativeScriptException(Isolate* isolate, TryCatch& tc,
                                              const std::string& message) {
+  // A caught termination has no exception value, message or stack to read:
+  // V8 hands back a sentinel, and formatting it would run JS on an isolate
+  // that must not run any.
+  if (tc.HasTerminated()) {
+    this->javascriptException_ = nullptr;
+    this->message_ = message;
+    this->name_ = "NativeScriptException";
+    return;
+  }
   Local<Value> error = tc.Exception();
   this->javascriptException_ = new Persistent<Value>(isolate, tc.Exception());
   this->message_ = GetErrorMessage(isolate, error, message);
@@ -333,7 +342,7 @@ static void ScheduleDeferredThrow(Isolate* isolate, NSException* e) {
   if (error->IsObject()) {
     auto errObject = error.As<Object>();
     auto fullMessageString = tns::ToV8String(isolate, "fullMessage");
-    if (errObject->HasOwnProperty(context, fullMessageString).ToChecked()) {
+    if (errObject->HasOwnProperty(context, fullMessageString).FromMaybe(false)) {
       // check if we have a "fullMessage" on the error, and log that instead - since it includes
       // more info about the exception.
       v8::Local<v8::Value> fullMessage_;
@@ -705,6 +714,14 @@ static bool GiveWorkerOnErrorAChance(Isolate* isolate, Local<Context> context, L
 }
 
 void NativeScriptException::ReThrowToV8(Isolate* isolate) {
+  // A materialized termination stays pending until the JS frames below
+  // unwind; throwing would replace it with an ordinary error. Only the V8
+  // signal counts here, not the runtime's termination-requested flag: while
+  // the request has not materialized, JS still runs, and an ordinary failure
+  // must still throw or the caller would continue with a bogus result.
+  if (isolate->IsExecutionTerminating()) {
+    return;
+  }
   @try {
     // The Isolate::Scope here is necessary because the Exception::Error method internally relies on
     // the Isolate::GetCurrent method which might return null if we do not use the proper scope
@@ -765,7 +782,7 @@ static bool GiveWorkerOnErrorAChance(Isolate* isolate, Local<Context> context, L
   std::string errMessage;
   bool hasFullErrorMessage = false;
   auto v8FullMessage = tns::ToV8String(isolate, "fullMessage");
-  if (error->IsObject() && error.As<Object>()->Has(context, v8FullMessage).ToChecked()) {
+  if (error->IsObject() && error.As<Object>()->Has(context, v8FullMessage).FromMaybe(false)) {
     hasFullErrorMessage = true;
     Local<Value> errMsgVal;
     bool success = error.As<Object>()->Get(context, v8FullMessage).ToLocal(&errMsgVal);