From 175944eaf543013e78b15bb13b8eacba86159247 Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Tue, 13 Oct 2026 11:53:08 -0700 Subject: [PATCH] Fix error reporting during worker termination Avoid constructing JavaScript error values after a Node-API environment starts terminating, and add a deterministic hosted-worker regression test.\n\nFixes microsoft/node-api-dotnet#505.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/NodeApi/JSError.cs | 62 ++++++++++++++++--- test/JSErrorTests.cs | 53 ++++++++++++++++ test/NativeAotTests.cs | 5 ++ test/TestCases/napi-dotnet/Errors.cs | 15 +++++ .../worker_termination_during_call.js | 31 ++++++++++ 5 files changed, 157 insertions(+), 9 deletions(-) create mode 100644 test/JSErrorTests.cs create mode 100644 test/TestCases/napi-dotnet/worker_termination_during_call.js diff --git a/src/NodeApi/JSError.cs b/src/NodeApi/JSError.cs index 33437ae9..e7998683 100644 --- a/src/NodeApi/JSError.cs +++ b/src/NodeApi/JSError.cs @@ -7,6 +7,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; +using Microsoft.JavaScript.NodeApi.Runtime; using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime; namespace Microsoft.JavaScript.NodeApi; @@ -142,7 +143,8 @@ public JSError(Exception exception) } } - _errorRef = CreateErrorReference(CreateErrorValueForException(exception, out _message)); + _message = GetExceptionMessage(exception); + _errorRef = CreateErrorReference(CreateErrorValueForException(exception, _message)); } public string Message @@ -190,11 +192,12 @@ public readonly JSValue Value } } - private static JSValue CreateErrorValueForException(Exception exception, out string message) - { - message = (exception as TargetInvocationException)?.InnerException?.Message + private static string GetExceptionMessage(Exception exception) + => (exception as TargetInvocationException)?.InnerException?.Message ?? exception.Message; + private static JSValue CreateErrorValueForException(Exception exception, string message) + { // If the exception is a JSException for an error value, use that error value; // otherwise construct a new error value from the exception message. JSValue error = (exception as JSException)?.Error?.Value ?? @@ -270,19 +273,60 @@ public readonly void ThrowError() /// public static void ThrowError(Exception exception) { - // Do not construct a JSError object here, because that would require a runtime context. - JSValue error = CreateErrorValueForException(exception, out string message); + JSRuntime runtime = JSValue.GetCurrentRuntime(out napi_env env); + + // If an exception is already pending, it will be propagated by Node when this callback + // returns. This also covers a terminating environment, where attempting any additional + // N-API work while constructing an error can throw out of the native callback boundary. + if (HasPendingExceptionOrIsClosing(runtime, env)) + { + return; + } + + string message = GetExceptionMessage(exception); + JSValue error; + try + { + // Do not construct a JSError object here, because that would require a runtime context. + error = CreateErrorValueForException(exception, message); + } + catch (JSException) + { + // Termination may begin after the check above, causing any of the N-API operations used + // to construct or augment the JS Error object to fail. Fall back to the direct runtime + // error API, which does not require creating or inspecting any JS values. + napi_status fallbackStatus = runtime.ThrowError(env, code: null, message); + if (fallbackStatus == napi_status.napi_ok || + fallbackStatus == napi_status.napi_pending_exception || + fallbackStatus == napi_status.napi_closing) + { + return; + } - napi_status status = error.Scope.Runtime.Throw( - (napi_env)JSValueScope.Current, (napi_value)error); + throw; + } - if (status != napi_status.napi_ok && status != napi_status.napi_pending_exception) + napi_status status = runtime.Throw(env, (napi_value)error); + + if (status != napi_status.napi_ok && + status != napi_status.napi_pending_exception && + status != napi_status.napi_closing) { throw new JSException( $"Failed to throw JS Error. Status: {status}\n{message}"); } } + private static bool HasPendingExceptionOrIsClosing(JSRuntime runtime, napi_env env) + { + // Use the raw runtime API instead of IsExceptionPending(), because ThrowIfFailed() would + // attempt to construct another JSError while this error-reporting path is already failing. + napi_status status = runtime.IsExceptionPending(env, out bool isPending); + return (status == napi_status.napi_ok && isPending) || + status == napi_status.napi_pending_exception || + status == napi_status.napi_closing; + } + public static void ThrowError(string message) => JSValue.GetCurrentRuntime(out napi_env env).ThrowError(env, code: null, message); diff --git a/test/JSErrorTests.cs b/test/JSErrorTests.cs new file mode 100644 index 00000000..f5b893bb --- /dev/null +++ b/test/JSErrorTests.cs @@ -0,0 +1,53 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using Microsoft.JavaScript.NodeApi.Runtime; +using Xunit; +using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime; + +namespace Microsoft.JavaScript.NodeApi.Test; + +public class JSErrorTests +{ + [Theory] + [InlineData(napi_status.napi_ok, true)] + [InlineData(napi_status.napi_pending_exception, false)] + [InlineData(napi_status.napi_closing, false)] + public void ThrowErrorReturnsWhenEnvironmentCannotAcceptAnException( + napi_status status, bool isPending) + { + var runtime = new TerminatingJSRuntime(status, isPending); + napi_env env = new(Environment.CurrentManagedThreadId); + using var scope = new JSValueScope( + JSValueScopeType.NoContext, + env, + runtime, + new MockJSRuntime.SynchronizationContext()); + + JSError.ThrowError(new InvalidOperationException("test")); + + Assert.Equal(1, runtime.IsExceptionPendingCallCount); + } + + private sealed class TerminatingJSRuntime : MockJSRuntime + { + private readonly napi_status _status; + private readonly bool _isPending; + + public TerminatingJSRuntime(napi_status status, bool isPending) + { + _status = status; + _isPending = isPending; + } + + public int IsExceptionPendingCallCount { get; private set; } + + public override napi_status IsExceptionPending(napi_env env, out bool result) + { + IsExceptionPendingCallCount++; + result = _isPending; + return _status; + } + } +} diff --git a/test/NativeAotTests.cs b/test/NativeAotTests.cs index 6ce5562a..07fcd15b 100644 --- a/test/NativeAotTests.cs +++ b/test/NativeAotTests.cs @@ -24,6 +24,11 @@ public class NativeAotTests // NativeAOT module has its own entry point that never calls it, so this case is only // meaningful under the hosted CLR host. See HostedClrTests. !testCaseName.Contains("/worker_teardown") && + + // This case reproduces termination of the hosted CoreCLR worker used by the + // node-api-dotnet package. Terminating a generated NativeAOT module while its unmanaged + // callback is still executing has separate fail-fast behavior outside this regression. + !testCaseName.Contains("/worker_termination_during_call") && !testCaseName.StartsWith("projects/", StringComparison.Ordinal)); [Theory] diff --git a/test/TestCases/napi-dotnet/Errors.cs b/test/TestCases/napi-dotnet/Errors.cs index 903d0460..3f7d3905 100644 --- a/test/TestCases/napi-dotnet/Errors.cs +++ b/test/TestCases/napi-dotnet/Errors.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.Threading; using System.Threading.Tasks; namespace Microsoft.JavaScript.NodeApi.TestCases; @@ -24,6 +25,14 @@ public static async Task ThrowAsyncDotnetError(string message) await Task.Yield(); throw new Exception(message); } + + public static void WaitAndThrow( + int milliseconds, IWorkerTerminationCallbacks workerTerminationCallbacks) + { + workerTerminationCallbacks.Started(); + Thread.Sleep(milliseconds); + throw new Exception("Worker termination test"); + } } [JSExport] @@ -31,3 +40,9 @@ public interface IJSErrors { void ThrowJSError(string message); } + +[JSExport] +public interface IWorkerTerminationCallbacks +{ + void Started(); +} diff --git a/test/TestCases/napi-dotnet/worker_termination_during_call.js b/test/TestCases/napi-dotnet/worker_termination_during_call.js new file mode 100644 index 00000000..9d9a4eb3 --- /dev/null +++ b/test/TestCases/napi-dotnet/worker_termination_during_call.js @@ -0,0 +1,31 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Regression test for terminating a Worker while it is executing a .NET call. +// +// The managed method calls back into JS to signal that it has started, then blocks before +// throwing. The parent terminates the Worker while the managed call is blocked. When the +// exception reaches JSError.ThrowError(Exception), the environment is already terminating. +// Attempting to construct another JS error in that state used to let a JSException escape the +// native callback boundary, aborting the entire process on Linux with exit code 134. + +const { Worker, isMainThread, parentPort } = require('worker_threads'); + +if (isMainThread) { + const worker = new Worker(__filename); + worker.on('error', (error) => { throw error; }); + worker.once('message', async (message) => { + if (message !== 'started') { + throw new Error(`Unexpected worker message: ${message}`); + } + + await worker.terminate(); + }); +} else { + const binding = require('../common').binding; + binding.Errors.waitAndThrow(250, { + started() { + parentPort.postMessage('started'); + }, + }); +}