Throw NotSupportedException for WaitHandle.WaitAny with 64 handle on STA threads...

This change is technically a breaking change, but it is better to throw than silently do the wrong thing.

Fixes #1646

diff --git a/src/coreclr/src/System.Private.CoreLib/Resources/Strings.resx b/src/coreclr/src/System.Private.CoreLib/Resources/Strings.resx
index add5d7a..e163e2f 100644 (file)
--- a/src/coreclr/src/System.Private.CoreLib/Resources/Strings.resx
+++ b/src/coreclr/src/System.Private.CoreLib/Resources/Strings.resx
@@ -2896,6 +2896,9 @@
   <data name="NotSupported_MaxWaitHandles" xml:space="preserve">
     <value>The number of WaitHandles must be less than or equal to 64.</value>
   </data>
+  <data name="NotSupported_MaxWaitHandles_STA" xml:space="preserve">
+    <value>The number of WaitHandles on a STA thread must be less than or equal to 63.</value>
+  </data>
   <data name="NotSupported_MemStreamNotExpandable" xml:space="preserve">
     <value>Memory stream is not expandable.</value>
   </data>

diff --git a/src/coreclr/src/vm/threads.cpp b/src/coreclr/src/vm/threads.cpp
index 9affaf7..4624156 100644 (file)
--- a/src/coreclr/src/vm/threads.cpp
+++ b/src/coreclr/src/vm/threads.cpp
@@ -3203,22 +3203,11 @@ DWORD Thread::DoAppropriateWait(AppropriateWaitFunc func, void *args,
 //--------------------------------------------------------------------
 DWORD MsgWaitHelper(int numWaiters, HANDLE* phEvent, BOOL bWaitAll, DWORD millis, BOOL bAlertable)
 {
-    STATIC_CONTRACT_THROWS;
-    // The true contract for GC trigger should be the following.  But this puts a very strong restriction
-    // on contract for functions that call EnablePreemptiveGC.
-    //if (GetThread() && !ThreadStore::HoldingThreadStore(GetThread())) {GC_TRIGGERS;} else {GC_NOTRIGGER;}
-    STATIC_CONTRACT_GC_TRIGGERS;
+    STANDARD_VM_CONTRACT;
 
     DWORD flags = 0;
     DWORD dwReturn=WAIT_ABANDONED;
 
-    Thread* pThread = GetThread();
-    // If pThread is NULL, we'd better shut down.
-    if (pThread == NULL)
-        _ASSERTE (g_fEEShutDown);
-
-    DWORD lastError = 0;
-
     // If we're going to pump, we cannot use WAIT_ALL.  That's because the wait would
     // only be satisfied if a message arrives while the handles are signalled.  If we
     // want true WAIT_ALL, we need to fire up a different thread in the MTA and wait
@@ -3244,8 +3233,12 @@ DWORD MsgWaitHelper(int numWaiters, HANDLE* phEvent, BOOL bWaitAll, DWORD millis
     if (bAlertable)
         flags |= COWAIT_ALERTABLE;
 
-    HRESULT hr = S_OK;
-    hr = CoWaitForMultipleHandles(flags, millis, numWaiters, phEvent, &dwReturn);
+    // CoWaitForMultipleHandles does not support more than 63 handles. It returns RPC_S_CALLPENDING for more than 63 handles
+    // that is impossible to differentiate from timeout.
+    if (numWaiters > 63)
+        COMPlusThrow(kNotSupportedException, W("NotSupported_MaxWaitHandles_STA"));
+
+    HRESULT hr = CoWaitForMultipleHandles(flags, millis, numWaiters, phEvent, &dwReturn);
 
     if (hr == RPC_S_CALLPENDING)
     {
@@ -3259,13 +3252,9 @@ DWORD MsgWaitHelper(int numWaiters, HANDLE* phEvent, BOOL bWaitAll, DWORD millis
         dwReturn = WAIT_FAILED;
     }
     else
-{
+    {
         dwReturn += WAIT_OBJECT_0;  // success -- bias back
-                }
-
-    lastError = ::GetLastError();
-
-    ::SetLastError(lastError);
+    }
 
     return dwReturn;
 }
@@ -3277,11 +3266,7 @@ DWORD MsgWaitHelper(int numWaiters, HANDLE* phEvent, BOOL bWaitAll, DWORD millis
 DWORD Thread::DoAppropriateAptStateWait(int numWaiters, HANDLE* pHandles, BOOL bWaitAll,
                                          DWORD timeout, WaitMode mode)
 {
-    CONTRACTL {
-        THROWS;
-        GC_TRIGGERS;
-    }
-    CONTRACTL_END;
+    STANDARD_VM_CONTRACT;
 
     BOOL alertable = (mode & WaitMode_Alertable) != 0;
 

diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.cs
index 3dfe5b6..6909892 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.cs
@@ -261,15 +261,6 @@ namespace System.Threading
         {
             if (waitHandles.Length == 0)
             {
-                //
-                // Some history: in CLR 1.0 and 1.1, we threw ArgumentException in this case, which was correct.
-                // Somehow, in 2.0, this became ArgumentNullException.  This was not fixed until Silverlight 2,
-                // which went back to ArgumentException.
-                //
-                // Now we're in a bit of a bind.  Backward-compatibility requires us to keep throwing ArgumentException
-                // in CoreCLR, and ArgumentNullException in the desktop CLR.  This is ugly, but so is breaking
-                // user code.
-                //
                 throw new ArgumentException(SR.Argument_EmptyWaithandleArray, nameof(waitHandles));
             }
             if (waitHandles.Length > MaxWaitHandles)

diff --git a/src/libraries/System.Runtime/tests/System/Threading/WaitHandleTests.cs b/src/libraries/System.Runtime/tests/System/Threading/WaitHandleTests.cs
index debcaff..1432b7a 100644 (file)
--- a/src/libraries/System.Runtime/tests/System/Threading/WaitHandleTests.cs
+++ b/src/libraries/System.Runtime/tests/System/Threading/WaitHandleTests.cs
@@ -78,14 +78,39 @@ namespace System.Threading.Tests
             Assert.Equal(0, WaitHandle.WaitAny(wh));
         }
 
+        static ManualResetEvent[] CreateManualResetEvents(int length)
+        {
+            var handles = new ManualResetEvent[length];
+            for (int i = 0; i < handles.Length; i++)
+                handles[i] = new ManualResetEvent(true);
+            return handles;
+        }
+
+        [Fact]
+        public static void WaitAny_MaxHandles()
+        {
+            Assert.Equal(0, WaitHandle.WaitAny(CreateManualResetEvents(64)));
+            Assert.Throws<NotSupportedException>(() => WaitHandle.WaitAny(CreateManualResetEvents(65)));
+        }
+
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        [PlatformSpecific(TestPlatforms.Windows)]
+        public static void WaitAny_MaxHandles_STA()
+        {
+            Thread t = new Thread(() =>
+            {
+                Assert.Equal(0, WaitHandle.WaitAny(CreateManualResetEvents(63)));
+                Assert.Throws<NotSupportedException>(() => WaitHandle.WaitAny(CreateManualResetEvents(64)));
+            });
+            t.SetApartmentState(ApartmentState.STA);
+            t.Start();
+            t.Join();
+        }
+
         [Fact]
         public static void WaitAll()
         {
-            var handles = new ManualResetEvent[] {
-            new ManualResetEvent(true),
-            new ManualResetEvent(true),
-            new ManualResetEvent(true)
-        };
+            ManualResetEvent[] handles = CreateManualResetEvents(3);
 
             Assert.True(WaitHandle.WaitAll(handles));
             Assert.True(WaitHandle.WaitAll(handles, 1));