- fix JSSynchronizationContext_Send_Post_Items_Cancellation (#97832)

- simplify JSSynchronizationContext.Dispose - add finally for ThrowOnBlockingWaitOnJSInteropThread
dotnet · Feb 1, 2024 · 36a2a8d · 36a2a8d
1 parent 4f0216f
commit 36a2a8d
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 36 deletions.
diff --git a/...ces.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.cs b/...ces.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.cs
@@ -226,11 +226,17 @@ public static void CompleteTask(JSMarshalerArgument* arguments_buffer)
                 if (holder.CallbackReady != null)
                 {
                     var threadFlag = Monitor.ThrowOnBlockingWaitOnJSInteropThread;
-                    Monitor.ThrowOnBlockingWaitOnJSInteropThread = false;
-#pragma warning disable CA1416 // Validate platform compatibility
-                    holder.CallbackReady?.Wait();
-#pragma warning restore CA1416 // Validate platform compatibility
-                    Monitor.ThrowOnBlockingWaitOnJSInteropThread = threadFlag;
+                    try
+                    {
+                        Monitor.ThrowOnBlockingWaitOnJSInteropThread = false;
+    #pragma warning disable CA1416 // Validate platform compatibility
+                        holder.CallbackReady?.Wait();
+    #pragma warning restore CA1416 // Validate platform compatibility
+                    }
+                    finally
+                    {
+                        Monitor.ThrowOnBlockingWaitOnJSInteropThread = threadFlag;
+                    }
                 }
 #endif
                 var callback = holder.Callback!;

diff --git a/...ices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs b/...ices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs
@@ -90,12 +90,12 @@ internal void UninstallWebWorkerInterop()
 
             // this will runtimeKeepalivePop()
             // and later maybeExit() -> __emscripten_thread_exit()
+            // this will also call JSSynchronizationContext.Dispose() on this instance
             jsProxyContext.Dispose();
 
             JSProxyContext.CurrentThreadContext = null;
             JSProxyContext.ExecutionContext = null;
             _isRunning = false;
-            Dispose();
         }
 
         public JSSynchronizationContext(bool isMainThread, CancellationToken cancellationToken)
@@ -217,9 +217,15 @@ public override void Send(SendOrPostCallback d, object? state)
                 }
 
                 var threadFlag = Monitor.ThrowOnBlockingWaitOnJSInteropThread;
-                Monitor.ThrowOnBlockingWaitOnJSInteropThread = false;
-                signal.Wait();
-                Monitor.ThrowOnBlockingWaitOnJSInteropThread = threadFlag;
+                try
+                {
+                    Monitor.ThrowOnBlockingWaitOnJSInteropThread = false;
+                    signal.Wait();
+                }
+                finally
+                {
+                    Monitor.ThrowOnBlockingWaitOnJSInteropThread = threadFlag;
+                }
 
                 if (_isCancellationRequested)
                 {
@@ -280,25 +286,23 @@ private void Pump()
             }
         }
 
-        private void Dispose(bool disposing)
+
+        internal void Dispose()
         {
             if (!_isDisposed)
             {
-                if (disposing)
+                _isCancellationRequested = true;
+                Queue.Writer.TryComplete();
+                while (Queue.Reader.TryRead(out var item))
                 {
-                    Queue.Writer.TryComplete();
+                    // the Post is checking _isCancellationRequested after .Wait()
+                    item.Signal?.Set();
                 }
                 _isDisposed = true;
                 _cancellationTokenRegistration.Dispose();
                 previousSynchronizationContext = null;
             }
         }
-
-        internal void Dispose()
-        {
-            Dispose(disposing: true);
-            GC.SuppressFinalize(this);
-        }
     }
 }
 

diff --git a/...e.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSWebWorker.cs b/...e.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSWebWorker.cs
@@ -112,6 +112,9 @@ private void ThreadMain()
                         return;
                     }
 
+                    // JSSynchronizationContext also registers to _cancellationToken
+                    _jsSynchronizationContext = JSSynchronizationContext.InstallWebWorkerInterop(false, _cancellationToken);
+
                     // receive callback when the cancellation is requested
                     _cancellationRegistration = _cancellationToken.Register(static (o) =>
                     {
@@ -120,9 +123,6 @@ private void ThreadMain()
                         self.PropagateCompletionAndDispose(Task.FromCanceled<T>(self._cancellationToken));
                     }, this);
 
-                    // JSSynchronizationContext also registers to _cancellationToken
-                    _jsSynchronizationContext = JSSynchronizationContext.InstallWebWorkerInterop(false, _cancellationToken);
-
                     var childScheduler = TaskScheduler.FromCurrentSynchronizationContext();
 
                     // This code is exiting thread ThreadMain() before all promises are resolved.

diff --git a/...pServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs b/...pServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs
@@ -84,8 +84,18 @@ public async Task JSSynchronizationContext_Send_Post_Items_Cancellation()
                 capturedSynchronizationContext = SynchronizationContext.Current;
                 jswReady.SetResult();
 
-                // blocking the worker, so that JSSynchronizationContext could enqueue next tasks
-                blocker.Wait();
+                var threadFlag = Monitor.ThrowOnBlockingWaitOnJSInteropThread;
+                try
+                {
+                    Monitor.ThrowOnBlockingWaitOnJSInteropThread = false;
+                    
+                    // blocking the worker, so that JSSynchronizationContext could enqueue next tasks
+                    blocker.Wait();
+                }
+                finally
+                {
+                    Monitor.ThrowOnBlockingWaitOnJSInteropThread = threadFlag;
+                }
 
                 return never.Task;
             }, cts.Token);
@@ -98,35 +108,51 @@ public async Task JSSynchronizationContext_Send_Post_Items_Cancellation()
             var hitAfterPost = false;
             var hitAfterSend = false;
 
-            var canceledSend = Task.Run(async () =>
+            var canceledSend = Task.Run(() =>
             {
-                // this will be blocked until blocker.Set()
                 sendReady.SetResult();
-                await canceled.Task;
-                capturedSynchronizationContext.Send(_ =>
+                // this will be blocked until blocker.Set()
+                try
                 {
-                    // then it should get canceled and not executed
-                    shouldNotHitSend = true;
-                }, null);
+                    capturedSynchronizationContext.Send(_ =>
+                    {
+                        // then it should get canceled and not executed
+                        shouldNotHitSend = true;
+                    }, null);
+                }
+                catch (Exception ex)
+                {
+                    return Task.FromException(ex);
+                }
                 hitAfterSend = true;
-                return Task.CompletedTask;
+                return Task.FromException(new Exception("Should be unreachable"));
             });
 
             var canceledPost = Task.Run(() =>
             {
-                capturedSynchronizationContext.Post(_ =>
+                try
                 {
-                    // then it should get canceled and not executed
-                    shouldNotHitPost = true;
-                }, null);
+                    capturedSynchronizationContext.Post(_ =>
+                    {
+                        // then it should get canceled and not executed
+                        shouldNotHitPost = true;
+                    }, null);
+                }
+                catch (Exception ex)
+                {
+                    Console.WriteLine("Unexpected exception " + ex);
+                    postReady.SetException(ex);
+                    return Task.FromException(ex);
+                }
                 hitAfterPost = true;
                 postReady.SetResult();
                 return Task.CompletedTask;
             });
 
             // make sure that jobs got the chance to enqueue
-            await sendReady.Task;
             await postReady.Task;
+            await sendReady.Task;
+            await Task.Delay(200); // make sure that 
 
             // this could should be delivered immediately
             cts.Cancel();