Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle scope validation tests #180

Merged
merged 9 commits into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/NodeApi.DotNetHost/ManagedHost.cs
Original file line number Diff line number Diff line change
Expand Up @@ -393,14 +393,15 @@ public JSValue LoadModule(JSCallbackArgs args)
}
}

JSValueScope scope = JSValueScope.Current;
JSValue exports = JSValue.CreateObject();

var result = (napi_value?)initializeMethod.Invoke(
null, new object[] { (napi_env)JSValueScope.Current, (napi_value)exports });
null, new object[] { (napi_env)scope, (napi_value)exports });

if (result != null && result.Value != default)
{
exports = new JSValue(result.Value);
exports = new JSValue(result.Value, scope);
}

if (exports.IsObject())
Expand Down
17 changes: 10 additions & 7 deletions src/NodeApi.Generator/ModuleGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ public class ModuleGenerator : SourceGenerator, ISourceGenerator

public GeneratorExecutionContext Context { get; protected set; }

#pragma warning disable IDE0060 // Unused parameter
public void Initialize(GeneratorInitializationContext context)
#pragma warning restore IDE0060
{
// Note source generators cannot be directly launched in a debugger,
// because the generator runs at build time, not at application run-time.
Expand Down Expand Up @@ -280,6 +282,10 @@ private SourceBuilder GenerateModuleInitializer(
s += $"public static class {ModuleInitializerClassName}";
s += "{";

// The module scope is not disposed after a successful initialization. It becomes
// the parent of callback scopes, allowing the JS runtime instance to be inherited.
s += "private static JSValueScope _moduleScope;";

// The unmanaged entrypoint is used only when the AOT-compiled module is loaded.
s += "#if !NETFRAMEWORK";
s += $"[UnmanagedCallersOnly(EntryPoint = \"{ModuleRegisterFunctionName}\")]";
Expand All @@ -291,11 +297,11 @@ private SourceBuilder GenerateModuleInitializer(
// The main initialization entrypoint is called by the `ManagedHost`, and by the unmanaged entrypoint.
s += $"public static napi_value {ModuleInitializeMethodName}(napi_env env, napi_value exports)";
s += "{";
s += "var scope = new JSValueScope(JSValueScopeType.Module, env);";
s += "_moduleScope = new JSValueScope(JSValueScopeType.Module, env, runtime: default);";
s += "try";
s += "{";
s += "JSRuntimeContext context = scope.RuntimeContext;";
s += "JSValue exportsValue = new(exports, scope);";
s += "JSRuntimeContext context = _moduleScope.RuntimeContext;";
s += "JSValue exportsValue = new(exports, _moduleScope);";
s++;

if (moduleInitializer is IMethodSymbol moduleInitializerMethod)
Expand Down Expand Up @@ -325,15 +331,12 @@ private SourceBuilder GenerateModuleInitializer(
s += "return (napi_value)exportsValue;";
}

// The module scope is not disposed before a successful return. It becomes the parent
// of callback scopes, allowing the JS runtime instance to be inherited.

s += "}";
s += "catch (System.Exception ex)";
s += "{";
s += "System.Console.Error.WriteLine($\"Failed to export module: {ex}\");";
s += "JSError.ThrowError(ex);";
s += "scope.Dispose();";
s += "_moduleScope.Dispose();";
s += "return exports;";
s += "}";
s += "}";
Expand Down
9 changes: 6 additions & 3 deletions src/NodeApi.Generator/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -281,17 +281,20 @@ private static IEnumerable<string> SplitWithQuotes(string line)
{
StringBuilder s = new();
bool inQuotes = false;
bool foundQuotes = false;
foreach (char c in line)
{
if (c == '"')
{
inQuotes = !inQuotes;
foundQuotes = true;
}
else if (c == ' ' && !inQuotes)
{
if (s.Length > 0)
if (s.Length > 0 || foundQuotes)
{
yield return s.ToString();
foundQuotes = false;
s.Clear();
}
}
Expand All @@ -301,7 +304,7 @@ private static IEnumerable<string> SplitWithQuotes(string line)
}
}

if (s.Length > 0)
if (s.Length > 0 || foundQuotes)
{
yield return s.ToString();
}
Expand Down Expand Up @@ -344,7 +347,7 @@ private static void ResolveSystemAssemblies(
{
if (targetingPacks.Count == 0)
{
// If no targeting packs were specified, use the deafult targeting pack for .NET.
// If no targeting packs were specified, use the default targeting pack for .NET.
targetingPacks.Add("Microsoft.NETCore.App");
}

Expand Down
19 changes: 13 additions & 6 deletions src/NodeApi/DotNetHost/NativeHost.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@ internal unsafe partial class NativeHost : IDisposable
private static readonly string s_managedHostTypeName =
typeof(NativeHost).Namespace + ".ManagedHost";

private static JSRuntime? s_jsRuntime;
private string? _targetFramework;
private string? _managedHostPath;
private ICLRRuntimeHost* _runtimeHost;
private hostfxr_handle _hostContextHandle;
private readonly JSValueScope _hostScope;
private JSReference? _exports;

public static bool IsTracingEnabled { get; } =
Expand All @@ -48,15 +50,18 @@ public static napi_value InitializeModule(napi_env env, napi_value exports)
{
Trace($"> NativeHost.InitializeModule({env.Handle:X8}, {exports.Handle:X8})");

JSRuntime runtime = new NodejsRuntime();
using JSValueScope scope = new(JSValueScopeType.NoContext, env, runtime);
jasongin marked this conversation as resolved.
Show resolved Hide resolved
s_jsRuntime ??= new NodejsRuntime();

// The native host JSValueScope is not disposed after a successful initialization. It
// becomes the parent of callback scopes, allowing the JS runtime instance to be inherited.
JSValueScope hostScope = new(JSValueScopeType.NoContext, env, s_jsRuntime);
try
{
NativeHost host = new();
NativeHost host = new(hostScope);

// Do not use JSModuleBuilder here because it relies on having a current context.
// But the context will be set by the managed host.
new JSValue(exports, scope).DefineProperties(
new JSValue(exports, hostScope).DefineProperties(
// The package index.js will invoke the initialize method with the path to
// the managed host assembly.
JSPropertyDescriptor.Function("initialize", host.InitializeManagedHost));
Expand All @@ -65,16 +70,18 @@ public static napi_value InitializeModule(napi_env env, napi_value exports)
{
string message = $"Failed to load CLR native host module: {ex}";
Trace(message);
runtime.Throw(env, (napi_value)JSValue.CreateError(null, (JSValue)message));
s_jsRuntime.Throw(env, (napi_value)JSValue.CreateError(null, (JSValue)message));
hostScope.Dispose();
}

Trace("< NativeHost.InitializeModule()");

return exports;
}

public NativeHost()
private NativeHost(JSValueScope hostScope)
{
_hostScope = hostScope;
}

/// <summary>
Expand Down
29 changes: 9 additions & 20 deletions src/NodeApi/Interop/JSRuntimeContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.IO;
using System.Runtime.InteropServices;
using System.Threading;
using Microsoft.JavaScript.NodeApi.Runtime;
using static Microsoft.JavaScript.NodeApi.Interop.JSCollectionProxies;
using static Microsoft.JavaScript.NodeApi.JSNativeApi;
using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime;
Expand Down Expand Up @@ -118,13 +119,19 @@ public static explicit operator JSRuntimeContext(napi_env env)
/// thread.</exception>
public static JSRuntimeContext Current => JSValueScope.Current.RuntimeContext;

public JSRuntime Runtime { get; }

public JSSynchronizationContext SynchronizationContext { get; }

public JSRuntimeContext(napi_env env)
internal JSRuntimeContext(
napi_env env,
JSRuntime runtime,
JSSynchronizationContext? synchronizationContext = null)
{
_env = env;
Runtime = runtime;
SetInstanceData(env, this);
SynchronizationContext = JSSynchronizationContext.Create();
SynchronizationContext = synchronizationContext ?? JSSynchronizationContext.Create();
}

/// <summary>
Expand Down Expand Up @@ -692,22 +699,4 @@ internal void FreeGCHandle(GCHandle handle)

handle.Free();
}

/// <summary>
/// Frees a GC handle previously allocated via <see cref="AllocGCHandle(object)" />
/// and tracked on the runtime context obtained from environment instance data.
/// </summary>
/// <exception cref="InvalidOperationException">The handle was not previously allocated
/// by <see cref="AllocGCHandle(object)" />, or was already freed.</exception>
internal static void FreeGCHandle(GCHandle handle, napi_env env)
{
if (GetInstanceData(env) is JSRuntimeContext runtimeContext)
{
runtimeContext.FreeGCHandle(handle);
}
else
{
handle.Free();
}
}
}
25 changes: 22 additions & 3 deletions src/NodeApi/Interop/JSSynchronizationContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,25 @@

namespace Microsoft.JavaScript.NodeApi.Interop;

/// <summary>
/// Manages the synchronization context for a JavaScript environment, allowing callbacks and
/// asynchronous continuations to be invoked on the JavaScript thread that runs the environment.
/// </summary>
/// <remarks>
/// All JavaScript values are bound to the thread that runs the JS environment and can only be
/// accessed from the same thread. Attempts to access a JavaScript value from a different thread
/// will throw <see cref="JSInvalidScopeException" />.
jasongin marked this conversation as resolved.
Show resolved Hide resolved
/// <para/>
/// Use of <see cref="Task.ConfigureAwait(bool)"/> with <c>continueOnCapturedContext:false</c>
/// can prevent execution from returning to the JS thread, though it isn't necessarily a problem
/// as long as there is a top-level continuation that uses <c>continueOnCapturedContext:true</c>
/// (the default) to return to the JS thread.
/// <para/>
/// Code that makes explicit use of .NET threads or thread pools may need to capture the
/// <see cref="JSSynchronizationContext.Current" /> context (before switching off the JS thread)
/// and hold it for later use to call back to JS via <see cref="JSSynchronizationContext.Post"/>,
/// <see cref="JSSynchronizationContext.Run"/>, or <see cref="JSSynchronizationContext.RunAsync"/>.
/// </remarks>
public abstract class JSSynchronizationContext : SynchronizationContext, IDisposable
{
public bool IsDisposed { get; private set; }
Expand Down Expand Up @@ -224,7 +243,7 @@ public Task<T> RunAsync<T>(Func<Task<T>> asyncAction)
}
}

public sealed class JSTsfnSynchronizationContext : JSSynchronizationContext
internal sealed class JSTsfnSynchronizationContext : JSSynchronizationContext
{
private readonly JSThreadSafeFunction _tsfn;

Expand All @@ -233,7 +252,7 @@ public JSTsfnSynchronizationContext()
_tsfn = new JSThreadSafeFunction(
maxQueueSize: 0,
initialThreadCount: 1,
asyncResourceName: (JSValue)"SynchronizationContext");
asyncResourceName: (JSValue)nameof(JSSynchronizationContext));

// Unref TSFN to indicate that this TSFN is not preventing Node.JS shutdown.
_tsfn.Unref();
Expand Down Expand Up @@ -295,7 +314,7 @@ public override void Send(SendOrPostCallback callback, object? state)
}
}

public sealed class JSDispatcherSynchronizationContext : JSSynchronizationContext
internal sealed class JSDispatcherSynchronizationContext : JSSynchronizationContext
{
private readonly JSDispatcherQueue _queue;

Expand Down
7 changes: 5 additions & 2 deletions src/NodeApi/Interop/JSThreadSafeFunction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ private static unsafe void CustomCallJS(napi_env env, napi_value jsCallback, nin

try
{
using JSValueScope scope = new(JSValueScopeType.Callback, env);
using JSValueScope scope = new(JSValueScopeType.Callback, env, runtime: null);

object? callbackData = null;
if (data != default)
Expand Down Expand Up @@ -267,7 +267,7 @@ private static unsafe void DefaultCallJS(napi_env env, napi_value jsCallback, ni

try
{
using JSValueScope scope = new(JSValueScopeType.Callback, env);
using JSValueScope scope = new(JSValueScopeType.Callback, env, runtime: null);

if (data != default)
{
Expand Down Expand Up @@ -299,6 +299,9 @@ private static unsafe void DefaultCallJS(napi_env env, napi_value jsCallback, ni
}
catch (Exception ex)
{
#if DEBUG
Console.Error.WriteLine(ex);
#endif
JSError.Fatal(ex.Message);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/NodeApi/JSException.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Microsoft.JavaScript.NodeApi;

/// <summary>
/// An exception that was caused by an error thrown by JavaScript code or
/// interactions with the JavaScript engine.
/// interactions with JavaScript objects.
/// </summary>
public class JSException : Exception
{
Expand Down
87 changes: 87 additions & 0 deletions src/NodeApi/JSInvalidScopeException.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System;
using System.Threading;
using Microsoft.JavaScript.NodeApi.Interop;

namespace Microsoft.JavaScript.NodeApi;

/// <summary>
/// An exception that was caused by an attempt to access a JavaScript value without any
/// <see cref="JSValueScope" /> established on the current thread, or from a thread associated
/// with a different environment / root scope.
/// </summary>
/// <remarks>
/// All JavaScript values are created within a scope that is bound to the thread that runs the
/// JS environment. They can only be accessed from the same thread and only as long as the scope
/// is still valid (not disposed).
/// </remarks>
/// <seealso cref="JSSynchronizationContext"/>
public class JSInvalidScopeException : InvalidOperationException
{
/// <summary>
/// Creates a new instance of <see cref="JSInvalidScopeException" /> with a
/// current scope and message.
/// </summary>
public JSInvalidScopeException(
JSValueScope? currentScope,
vmoroz marked this conversation as resolved.
Show resolved Hide resolved
string? message = null)
: this(currentScope, targetScope: null, message)
{
}

/// <summary>
/// Creates a new instance of <see cref="JSInvalidScopeException" /> with current
/// and target scopes and a message.
/// </summary>
public JSInvalidScopeException(
JSValueScope? currentScope,
JSValueScope? targetScope,
string? message = null)
: base(message ?? GetMessage(currentScope, targetScope))
{
CurrentScope = currentScope;
TargetScope = targetScope;
}

/// <summary>
/// Gets the scope associated with the current thread (<see cref="JSValueScope.Current" />)
/// when the exception was thrown, or null if there was no scope for the thread.
/// </summary>
public JSValueScope? CurrentScope { get; internal set; }

/// <summary>
/// Gets the scope of the value (<see cref="JSValue.Scope" />) that was being accessed when
/// the exception was thrown, or null if a static operation was attempted.
/// </summary>
public JSValueScope? TargetScope { get; internal set; }

private static string GetMessage(JSValueScope? currentScope, JSValueScope? targetScope)
{
int threadId = Environment.CurrentManagedThreadId;
string? threadName = Thread.CurrentThread.Name;
string threadDescription = string.IsNullOrEmpty(threadName) ?
$"#{threadId}" : $"#{threadId} \"{threadName}\"";

if (targetScope == null)
{
// If the target scope is null, then this was an attempt to access either a static
// operation or a JS reference (which has an environment but no scope).
if (currentScope != null)
{
// In that case if the current scope is NOT null this exception
// shouldn't be thrown.
throw new ArgumentException("Current scope must be null if target scope is null.");
}

return $"There is no active JS value scope.\nCurrent thread: {threadDescription}. " +
$"Consider using {nameof(JSSynchronizationContext)} to switch to the JS thread.";
}

return "The JS value scope cannot be accessed from the current thread.\n" +
$"The scope of type {targetScope.ScopeType} was created on thread" +
$"#{targetScope.ThreadId} and is being accessed from {threadDescription}. " +
$"Consider using {nameof(JSSynchronizationContext)} to switch to the JS thread.";
}
}
Loading