[wasm][mt] Improve blocking wait detection and tests (#98045)

* [wasm][mt] Improve blocking wait detection and tests Add check to the `SemaphoreSlim.Wait`. The wait will eventually continue to the `Monitor.Wait`, it can spin wait first though and return, so catch it earlier to avoid non-deterministic behavior. Improve the blocking wait detection and extract it to the new method. Also introduce helper method to force a blocking wait in places we want it on the JS interop threads. * Feedback
dotnet · Feb 7, 2024 · 59831ac · 59831ac
1 parent 0b96be1
commit 59831ac
Show file tree

Hide file tree

Showing 12 changed files with 72 additions and 45 deletions.
diff --git a/src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ExtraApis.cs b/src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ExtraApis.cs
@@ -41,10 +41,13 @@ public static partial class Debug
 #if FEATURE_WASM_MANAGED_THREADS
 namespace System.Threading
 {
-    public partial class Monitor
+    public partial class Thread
     {
         [ThreadStatic]
         public static bool ThrowOnBlockingWaitOnJSInteropThread;
+
+        public static void AssureBlockingPossible() { throw null; }
+        public static void ForceBlockingWait(Action<object?> action, object? state) { throw null; }
     }
 }
 #endif
diff --git a/src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ExtraApis.txt b/src/libraries/System.Private.CoreLib/ref/System.Private.CoreLib.ExtraApis.txt
@@ -5,4 +5,6 @@ T:System.Runtime.Serialization.DeserializationToken
 M:System.Runtime.Serialization.SerializationInfo.StartDeserialization
 T:System.Diagnostics.DebugProvider
 M:System.Diagnostics.Debug.SetProvider(System.Diagnostics.DebugProvider)
-F:System.Threading.Monitor.ThrowOnBlockingWaitOnJSInteropThread
+M:System.Threading.Thread.AssureBlockingPossible
+F:System.Threading.Thread.ThrowOnBlockingWaitOnJSInteropThread
+F:System.Threading.Thread.ForceBlockingWait
diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
@@ -4298,4 +4298,7 @@
   <data name="NotSupported_AssemblySave" xml:space="preserve">
     <value>This AssemblyBuilder instance doesn't support saving. Use AssemblyBuilder.DefinePersistedAssembly to create an AssemblyBuilder instance that supports saving.</value>
   </data>
+  <data name="WasmThreads_BlockingWaitNotSupportedOnJSInterop" xml:space="preserve">
+    <value>Blocking wait is not supported on the JS interop threads.</value>
+  </data>
 </root>
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs
@@ -285,6 +285,9 @@ public bool Wait(int millisecondsTimeout)
         public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)
         {
             CheckDispose();
+#if FEATURE_WASM_MANAGED_THREADS
+            Thread.AssureBlockingPossible();
+#endif
 
             if (millisecondsTimeout < -1)
             {

diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs
@@ -671,5 +671,33 @@ public static int GetCurrentProcessorId()
         // a speed check will determine refresh rate of the cache and will report if caching is not advisable.
         // we will record that in a readonly static so that it could become a JIT constant and bypass caching entirely.
         private static readonly bool s_isProcessorNumberReallyFast = ProcessorIdCache.ProcessorNumberSpeedCheck();
+
+#if FEATURE_WASM_MANAGED_THREADS
+        [ThreadStatic]
+        public static bool ThrowOnBlockingWaitOnJSInteropThread;
+
+        public static void AssureBlockingPossible()
+        {
+            if (ThrowOnBlockingWaitOnJSInteropThread)
+            {
+                throw new PlatformNotSupportedException(SR.WasmThreads_BlockingWaitNotSupportedOnJSInterop);
+            }
+        }
+
+        public static void ForceBlockingWait(Action<object?> action, object? state = null)
+        {
+            var flag = ThrowOnBlockingWaitOnJSInteropThread;
+            try
+            {
+                ThrowOnBlockingWaitOnJSInteropThread = false;
+
+                action(state);
+            }
+            finally
+            {
+                ThrowOnBlockingWaitOnJSInteropThread = flag;
+            }
+        }
+#endif
     }
 }
diff --git a/...ces.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.cs b/...ces.JavaScript/src/System/Runtime/InteropServices/JavaScript/Interop/JavaScriptExports.cs
@@ -233,18 +233,9 @@ public static void CompleteTask(JSMarshalerArgument* arguments_buffer)
 
                 if (holder.CallbackReady != null)
                 {
-                    var threadFlag = Monitor.ThrowOnBlockingWaitOnJSInteropThread;
-                    try
-                    {
-                        Monitor.ThrowOnBlockingWaitOnJSInteropThread = false;
 #pragma warning disable CA1416 // Validate platform compatibility
-                        holder.CallbackReady?.Wait();
+                    Thread.ForceBlockingWait(static (callbackReady) => ((ManualResetEventSlim)callbackReady!).Wait(), holder.CallbackReady);
 #pragma warning restore CA1416 // Validate platform compatibility
-                    }
-                    finally
-                    {
-                        Monitor.ThrowOnBlockingWaitOnJSInteropThread = threadFlag;
-                    }
                 }
 
                 lock (ctx)

diff --git a/...ices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs b/...ices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSSynchronizationContext.cs
@@ -65,7 +65,7 @@ public static JSSynchronizationContext InstallWebWorkerInterop(bool isMainThread
             // - synchronous [JSExport] into managed code, which would block
             // - synchronous [JSImport] to another thread, which would block
             // see also https://github.com/dotnet/runtime/issues/76958#issuecomment-1921418290
-            Monitor.ThrowOnBlockingWaitOnJSInteropThread = true;
+            Thread.ThrowOnBlockingWaitOnJSInteropThread = true;
 
             var proxyContext = ctx.ProxyContext;
             JSProxyContext.CurrentThreadContext = proxyContext;
@@ -216,11 +216,8 @@ public override void Send(SendOrPostCallback d, object? state)
                 d(state);
                 return;
             }
-            // TODO, refactor into single assert method
-            if (Monitor.ThrowOnBlockingWaitOnJSInteropThread)
-            {
-                throw new PlatformNotSupportedException("Blocking wait is not supported on the JS interop threads.");
-            }
+
+            Thread.AssureBlockingPossible();
 
             using (var signal = new ManualResetEventSlim(false))
             {

diff --git a/...pServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs b/...pServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs
@@ -85,18 +85,8 @@ public async Task JSSynchronizationContext_Send_Post_Items_Cancellation()
                 capturedSynchronizationContext = SynchronizationContext.Current;
                 jswReady.SetResult();
 
-                var threadFlag = Monitor.ThrowOnBlockingWaitOnJSInteropThread;
-                try
-                {
-                    Monitor.ThrowOnBlockingWaitOnJSInteropThread = false;
-
-                    // blocking the worker, so that JSSynchronizationContext could enqueue next tasks
-                    blocker.Wait();
-                }
-                finally
-                {
-                    Monitor.ThrowOnBlockingWaitOnJSInteropThread = threadFlag;
-                }
+                // blocking the worker, so that JSSynchronizationContext could enqueue next tasks
+                Thread.ForceBlockingWait(static (b) => ((ManualResetEventSlim)b).Wait(), blocker);
 
                 return never.Task;
             }, cts.Token);
@@ -453,9 +443,12 @@ public async Task WaitAssertsOnJSInteropThreads(Executor executor, NamedCall met
             await executor.Execute(Task () =>
             {
                 Exception? exception = null;
-                try {
+                try
+                {
                     method.Call(cts.Token);
-                } catch (Exception ex) {
+                }
+                catch (Exception ex)
+                {
                     exception = ex;
                 }
 

diff --git a/...vices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTestBase.cs b/...vices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTestBase.cs
@@ -158,6 +158,13 @@ public class NamedCall
                         mr.Wait(cts.Token);
                     } catch (OperationCanceledException) { /* ignore */ }
                 }},
+                new NamedCall { Name = "SemaphoreSlim.Wait", Call = delegate (CancellationToken ct) {
+                    using var sem = new SemaphoreSlim(2);
+                    var cts = new CancellationTokenSource(8);
+                    try {
+                        sem.Wait(cts.Token);
+                    } catch (OperationCanceledException) { /* ignore */ }
+                }},
         };
 
         public static IEnumerable<object[]> GetTargetThreadsAndBlockingCalls()

diff --git a/src/libraries/System.Threading.Thread/src/CompatibilitySuppressions.Threading.xml b/src/libraries/System.Threading.Thread/src/CompatibilitySuppressions.Threading.xml
@@ -16,4 +16,16 @@
     <DiagnosticId>CP0014</DiagnosticId>
     <Target>M:System.Threading.Thread.UnsafeStart(System.Object):[T:System.Runtime.Versioning.UnsupportedOSPlatformAttribute]</Target>
   </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>F:System.Threading.Thread.ThrowOnBlockingWaitOnJSInteropThread</Target>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>M:System.Threading.Thread.AssureBlockingPossible</Target>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>M:System.Threading.Thread.ForceBlockingWait(System.Action{System.Object},System.Object)</Target>
+  </Suppression>
 </Suppressions>
diff --git a/src/libraries/System.Threading/src/CompatibilitySuppressions.Threading.xml b/src/libraries/System.Threading/src/CompatibilitySuppressions.Threading.xml
@@ -100,8 +100,4 @@
     <DiagnosticId>CP0014</DiagnosticId>
     <Target>M:System.Threading.Monitor.Wait(System.Object):[T:System.Runtime.Versioning.UnsupportedOSPlatformAttribute]</Target>
   </Suppression>
-  <Suppression>
-    <DiagnosticId>CP0002</DiagnosticId>
-    <Target>F:System.Threading.Monitor.ThrowOnBlockingWaitOnJSInteropThread</Target>
-  </Suppression>
 </Suppressions>
diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Monitor.Mono.cs
@@ -9,11 +9,6 @@ namespace System.Threading
 {
     public static partial class Monitor
     {
-#if FEATURE_WASM_MANAGED_THREADS
-        [ThreadStatic]
-        public static bool ThrowOnBlockingWaitOnJSInteropThread;
-#endif
-
         [Intrinsic]
         [MethodImplAttribute(MethodImplOptions.InternalCall)] // Interpreter is missing this intrinsic
         public static void Enter(object obj) => Enter(obj);
@@ -83,10 +78,7 @@ public static bool Wait(object obj, int millisecondsTimeout)
         {
             ArgumentNullException.ThrowIfNull(obj);
 #if FEATURE_WASM_MANAGED_THREADS
-            if (ThrowOnBlockingWaitOnJSInteropThread)
-            {
-                throw new PlatformNotSupportedException("blocking Wait is not supported on the JS interop threads.");
-            }
+            Thread.AssureBlockingPossible();
 #endif
             return ObjWait(millisecondsTimeout, obj);
         }