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

[browser][mt] Release all proxies of C# and JS objects on WebWorker #88052

Merged
merged 14 commits into from
Jul 24, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ private async Task CancelationHelper(Task jsTask, CancellationToken cancellation
FastState = WebSocketState.Aborted;
throw new OperationCanceledException(cancellationToken);
}
if (ex.Message == "OperationCanceledException")
if (ex.Message == "Error: OperationCanceledException")
{
FastState = WebSocketState.Aborted;
throw new OperationCanceledException("The operation was cancelled.", ex, cancellationToken);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,8 @@ public static void ReleaseJSOwnedObjectByGCHandle(JSMarshalerArgument* arguments
{
GCHandle handle = (GCHandle)arg_1.slot.GCHandle;

lock (JSHostImplementation.s_gcHandleFromJSOwnedObject)
{
JSHostImplementation.s_gcHandleFromJSOwnedObject.Remove(handle.Target!);
handle.Free();
}
JSHostImplementation.ThreadJsOwnedObjects.Remove(handle.Target!);
handle.Free();
}
catch (Exception ex)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ public static JSFunctionBinding BindManagedFunction(string fullyQualifiedName, i
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static unsafe void InvokeJSImpl(JSObject jsFunction, Span<JSMarshalerArgument> arguments)
{
ObjectDisposedException.ThrowIf(jsFunction.IsDisposed, jsFunction);
#if FEATURE_WASM_THREADS
JSObject.AssertThreadAffinity(jsFunction);
#endif
Expand Down Expand Up @@ -205,10 +206,7 @@ internal static unsafe void InvokeImportImpl(IntPtr fnHandle, Span<JSMarshalerAr
internal static unsafe JSFunctionBinding BindJSFunctionImpl(string functionName, string moduleName, ReadOnlySpan<JSMarshalerType> signatures)
{
#if FEATURE_WASM_THREADS
if (JSSynchronizationContext.CurrentJSSynchronizationContext == null)
{
throw new InvalidOperationException("Please use dedicated worker for working with JavaScript interop. See https://github.com/dotnet/runtime/blob/main/src/mono/wasm/threads.md#JS-interop-on-dedicated-threads ");
}
JSSynchronizationContext.AssertWebWorkerContext();
#endif

var signature = JSHostImplementation.GetMethodSignature(signatures);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ public static JSObject GlobalThis
{
get
{
#if FEATURE_WASM_THREADS
JSSynchronizationContext.AssertWebWorkerContext();
#endif
return JavaScriptImports.GetGlobalThis();
}
}
Expand All @@ -32,6 +35,9 @@ public static JSObject DotnetInstance
{
get
{
#if FEATURE_WASM_THREADS
JSSynchronizationContext.AssertWebWorkerContext();
#endif
return JavaScriptImports.GetDotnetInstance();
}
}
Expand All @@ -47,6 +53,9 @@ public static JSObject DotnetInstance
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Task<JSObject> ImportAsync(string moduleName, string moduleUrl, CancellationToken cancellationToken = default)
{
#if FEATURE_WASM_THREADS
JSSynchronizationContext.AssertWebWorkerContext();
#endif
return JSHostImplementation.ImportAsync(moduleName, moduleUrl, cancellationToken);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,28 @@ public static Dictionary<int, WeakReference<JSObject>> ThreadCsOwnedObjects
}

// we use this to maintain identity of GCHandle for a managed object
public static Dictionary<object, IntPtr> s_gcHandleFromJSOwnedObject = new Dictionary<object, IntPtr>(ReferenceEqualityComparer.Instance);
#if FEATURE_WASM_THREADS
[ThreadStatic]
#endif
private static Dictionary<object, IntPtr>? s_jsOwnedObjects;

public static Dictionary<object, IntPtr> ThreadJsOwnedObjects
{
get
{
s_jsOwnedObjects ??= new Dictionary<object, IntPtr>(ReferenceEqualityComparer.Instance);
return s_jsOwnedObjects;
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void ReleaseCSOwnedObject(nint jsHandle)
{
if (jsHandle != IntPtr.Zero)
{
#if FEATURE_WASM_THREADS
JSSynchronizationContext.AssertWebWorkerContext();
#endif
ThreadCsOwnedObjects.Remove((int)jsHandle);
Interop.Runtime.ReleaseCSOwnedObject(jsHandle);
}
Expand Down Expand Up @@ -66,18 +81,14 @@ public static IntPtr GetJSOwnedObjectGCHandle(object obj, GCHandleType handleTyp
if (obj == null)
return IntPtr.Zero;

IntPtr result;
lock (s_gcHandleFromJSOwnedObject)
{
IntPtr gcHandle;
if (s_gcHandleFromJSOwnedObject.TryGetValue(obj, out gcHandle))
if (ThreadJsOwnedObjects.TryGetValue(obj, out gcHandle))
return gcHandle;

result = (IntPtr)GCHandle.Alloc(obj, handleType);
s_gcHandleFromJSOwnedObject[obj] = result;
IntPtr result = (IntPtr)GCHandle.Alloc(obj, handleType);
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
ThreadJsOwnedObjects[obj] = result;
return result;
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static RuntimeMethodHandle GetMethodHandleFromIntPtr(IntPtr ptr)
Expand Down Expand Up @@ -188,6 +199,9 @@ public static unsafe void FreeMethodSignatureBuffer(JSFunctionBinding signature)

public static JSObject CreateCSOwnedProxy(nint jsHandle)
{
#if FEATURE_WASM_THREADS
JSSynchronizationContext.AssertWebWorkerContext();
#endif
JSObject? res;

if (!ThreadCsOwnedObjects.TryGetValue((int)jsHandle, out WeakReference<JSObject>? reference) ||
Expand Down Expand Up @@ -244,14 +258,48 @@ public static void InstallWebWorkerInterop(bool installJSSynchronizationContext,

public static void UninstallWebWorkerInterop()
{
var ctx = SynchronizationContext.Current as JSSynchronizationContext;
var ctx = JSSynchronizationContext.CurrentJSSynchronizationContext;
var uninstallJSSynchronizationContext = ctx != null;
if (uninstallJSSynchronizationContext)
{
foreach (var jsObjectWeak in ThreadCsOwnedObjects.Values)
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
{
if (jsObjectWeak.TryGetTarget(out var jso))
{
jso.Dispose();
}
}
foreach (var gch in ThreadJsOwnedObjects.Values)
{
GCHandle gcHandle = (GCHandle)gch;

// if this is pending promise we reject it
if (gcHandle.Target is TaskCallback holder)
{
unsafe
{
holder.Callback!.Invoke(null);
lambdageek marked this conversation as resolved.
Show resolved Hide resolved
}
}
gcHandle.Free();
}
SynchronizationContext.SetSynchronizationContext(ctx!.previousSynchronizationContext);
JSSynchronizationContext.CurrentJSSynchronizationContext = null;
ctx.isDisposed = true;
}
else
{
if (ThreadCsOwnedObjects.Count > 0)
{
throw new InvalidOperationException($"There should be no JSObjects on this thread");
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
}
if (ThreadJsOwnedObjects.Count > 0)
{
throw new InvalidOperationException($"There should be no JSObjects on this thread");
}
}
ThreadCsOwnedObjects.Clear();
ThreadJsOwnedObjects.Clear();
Interop.Runtime.UninstallWebWorkerInterop(uninstallJSSynchronizationContext);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ public partial class JSObject : IDisposable
public bool HasProperty(string propertyName)
{
ObjectDisposedException.ThrowIf(IsDisposed, this);
#if FEATURE_WASM_THREADS
JSObject.AssertThreadAffinity(this);
#endif
return JavaScriptImports.HasProperty(this, propertyName);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,16 @@ internal JSSynchronizationContext(Thread targetThread, IntPtr targetThreadId)
{
}

internal static void AssertWebWorkerContext()
{
#if FEATURE_WASM_THREADS
if (CurrentJSSynchronizationContext == null)
{
throw new InvalidOperationException("Please use dedicated worker for working with JavaScript interop. See https://github.com/dotnet/runtime/blob/main/src/mono/wasm/threads.md#JS-interop-on-dedicated-threads ");
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
}
#endif
}

private JSSynchronizationContext(Thread targetThread, IntPtr targetThreadId, WorkItemQueueType queue)
{
TargetThread = targetThread;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ public unsafe void ToManaged(out Task? value)
TaskCompletionSource tcs = new TaskCompletionSource(holder);
JSHostImplementation.ToManagedCallback callback = (JSMarshalerArgument* arguments_buffer) =>
{
if (arguments_buffer == null)
{
tcs.TrySetException(new TaskCanceledException("WebWorker which is origin of the Promise is being terminated."));
return;
}
ref JSMarshalerArgument arg_2 = ref arguments_buffer[3]; // set by caller when this is SetException call
// arg_3 set by caller when this is SetResult call, un-used here
if (arg_2.slot.Type != MarshalerType.None)
Expand Down Expand Up @@ -88,6 +93,12 @@ public unsafe void ToManaged<T>(out Task<T>? value, ArgumentToManagedCallback<T>
TaskCompletionSource<T> tcs = new TaskCompletionSource<T>(holder);
JSHostImplementation.ToManagedCallback callback = (JSMarshalerArgument* arguments_buffer) =>
{
if (arguments_buffer == null)
{
tcs.TrySetException(new TaskCanceledException("WebWorker which is origin of the Promise is being terminated."));
return;
}

ref JSMarshalerArgument arg_2 = ref arguments_buffer[3]; // set by caller when this is SetException call
ref JSMarshalerArgument arg_3 = ref arguments_buffer[4]; // set by caller when this is SetResult call
if (arg_2.slot.Type != MarshalerType.None)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
namespace System.Runtime.InteropServices.JavaScript
{
/// <summary>
/// This is draft for possible public API of SynchronizationContext
/// Extensions of SynchronizationContext which propagate errors and return values
/// </summary>
public static class SynchronizationContextExtension
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@ public static Task<T> RunAsync<T>(Func<Task<T>> body, CancellationToken cancella
// the continuation is executed by setTimeout() callback of the thread.
res.ContinueWith(t =>
{
PostWhenDone(parentContext, tcs, res);
SendWhenDone(parentContext, tcs, res);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we changing these from post to send?

Copy link
Member Author

@pavelsavara pavelsavara Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It became bit more stable after this change, but I'm sure if we should go one or the other way. This is more conservative.

In case of JSException, receiving thread would have chance to print the JS stack trace on the receiving side before the webWorker is detached.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we will not include this class in public API of Net8, so we could continue improving it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that Send can deadlock in ways that Post cannot, as I understand it. So I'm wary of using it unless it's necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR only renames it. I agree with the concern.

Steve gave similar feedback on the Blazor PR dotnet/aspnetcore#48991 (comment)
They have interesting solution for error handling.

JSHostImplementation.UninstallWebWorkerInterop();
}, childScheduler);
}
catch (Exception ex)
{
PostWhenException(parentContext, tcs, ex);
SendWhenException(parentContext, tcs, ex);
}

});
Expand Down Expand Up @@ -75,13 +75,13 @@ public static Task RunAsyncVoid(Func<Task> body, CancellationToken cancellationT
// the continuation is executed by setTimeout() callback of the thread.
res.ContinueWith(t =>
{
PostWhenDone(parentContext, tcs, res);
SendWhenDone(parentContext, tcs, res);
JSHostImplementation.UninstallWebWorkerInterop();
}, childScheduler);
}
catch (Exception ex)
{
PostWhenException(parentContext, tcs, ex);
SendWhenException(parentContext, tcs, ex);
}

});
Expand Down Expand Up @@ -109,17 +109,17 @@ public static Task Run(Action body, CancellationToken cancellationToken)
try
{
body();
PostWhenDone(parentContext, tcs);
SendWhenDone(parentContext, tcs);
}
catch (Exception ex)
{
PostWhenException(parentContext, tcs, ex);
SendWhenException(parentContext, tcs, ex);
}
JSHostImplementation.UninstallWebWorkerInterop();
}
catch (Exception ex)
{
PostWhenException(parentContext, tcs, ex);
SendWhenException(parentContext, tcs, ex);
}

});
Expand All @@ -134,7 +134,7 @@ private static void PostWhenCancellation(SynchronizationContext ctx, TaskComplet
{
try
{
ctx.Post((_) => tcs.SetCanceled(), null);
ctx.Send((_) => tcs.SetCanceled(), null);
}
catch (Exception e)
{
Expand All @@ -146,19 +146,19 @@ private static void PostWhenCancellation<T>(SynchronizationContext ctx, TaskComp
{
try
{
ctx.Post((_) => tcs.SetCanceled(), null);
ctx.Send((_) => tcs.SetCanceled(), null);
}
catch (Exception e)
{
Environment.FailFast("WebWorker.RunAsync failed", e);
}
}

private static void PostWhenDone(SynchronizationContext ctx, TaskCompletionSource tcs, Task done)
private static void SendWhenDone(SynchronizationContext ctx, TaskCompletionSource tcs, Task done)
{
try
{
ctx.Post((_) =>
ctx.Send((_) =>
{
PropagateCompletion(tcs, done);
}, null);
Expand All @@ -169,47 +169,47 @@ private static void PostWhenDone(SynchronizationContext ctx, TaskCompletionSourc
}
}

private static void PostWhenDone(SynchronizationContext ctx, TaskCompletionSource tcs)
private static void SendWhenDone(SynchronizationContext ctx, TaskCompletionSource tcs)
{
try
{
ctx.Post((_) => tcs.SetResult(), null);
ctx.Send((_) => tcs.SetResult(), null);
}
catch (Exception e)
{
Environment.FailFast("WebWorker.RunAsync failed", e);
}
}

private static void PostWhenException(SynchronizationContext ctx, TaskCompletionSource tcs, Exception ex)
private static void SendWhenException(SynchronizationContext ctx, TaskCompletionSource tcs, Exception ex)
{
try
{
ctx.Post((_) => tcs.SetException(ex), null);
ctx.Send((_) => tcs.SetException(ex), null);
}
catch (Exception e)
{
Environment.FailFast("WebWorker.RunAsync failed", e);
}
}

private static void PostWhenException<T>(SynchronizationContext ctx, TaskCompletionSource<T> tcs, Exception ex)
private static void SendWhenException<T>(SynchronizationContext ctx, TaskCompletionSource<T> tcs, Exception ex)
{
try
{
ctx.Post((_) => tcs.SetException(ex), null);
ctx.Send((_) => tcs.SetException(ex), null);
}
catch (Exception e)
{
Environment.FailFast("WebWorker.RunAsync failed", e);
}
}

private static void PostWhenDone<T>(SynchronizationContext ctx, TaskCompletionSource<T> tcs, Task<T> done)
private static void SendWhenDone<T>(SynchronizationContext ctx, TaskCompletionSource<T> tcs, Task<T> done)
{
try
{
ctx.Post((_) =>
ctx.Send((_) =>
{
PropagateCompletion(tcs, done);
}, null);
Expand Down
Loading