[WaitHandle] Fix Wait(One|Any|All) and SignalAndWait with timeout

In the case where we would call for example WaitHandle.WaitOne with a timeout, we could run in the case where we would wait way longer than the timeout would allow it (240s instead of 90s for example). This would be due to spurious wake, which would not be considered to be an error, but which would lead calling the `mono_os_cond_timedwait` function with the same timeout every time. That would mean that `mono_os_cond_timedwait` would return that it timedout, if and only if, it would timed out after N milliseconds, with N the millisecondsTimeout parameter to WaitOne in this example. That mean that any spurious wake would effectively lead to rewaiting with a full timeout every time.

That could lead to the following scenario:
 - T1 calls waitHandle.WaitOne(timeout = 1000ms) at T=0ms
 - T1 calls mono_os_cond_timedwait(timeout = 1000) at T=0ms
 - GC suspend T1 at T=500ms <- here it's not necessarily the GC, it can be anything suspending/signaling the thread
 - T1 spurious wake, and call mono_os_cond_timedwait(timeout = 1000) at T=500ms
 - GC interrupt T1 at T=1250ms
 - T1 spurious wake, and call mono_os_cond_timedwait(timeout = 1000) at T=1250ms
 - etc.

As you can see, T1 should have returned from waitHandle.WaitOne before the second spurious wake, but as it recalled mono_os_cond_timedwait with a timeout of 1000ms again, it didn't.

The fix consists in keeping track of when the call is supposed to finish, and pass an adaptable timeout to mono_os_cond_timedwait, which decreases as time goes on, to enventually reach 0, and return in a timely manner.

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=38382

diff --git a/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs b/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs
index 5f8056e67ce9d0bed34ae4ee8bab8def7e131b31..122171b822a69f17d714e2899591739a8d4cda11 100644 (file)
--- a/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs
+++ b/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs
@@ -395,6 +395,94 @@ namespace MonoTests.System.Threading {
                        }
                }
 
+               [Test]
+               public void WaitOneWithTimeoutAndSpuriousWake ()
+               {
+                       /* This is to test that WaitEvent.WaitOne is not going to wait largely
+                        * more than its timeout. In this test, it shouldn't wait more than
+                        * 1500 milliseconds, with its timeout being 1000ms */
+
+                       using (ManualResetEvent mre = new ManualResetEvent (false))
+                       using (ManualResetEvent ready = new ManualResetEvent (false)) {
+                               var thread = new Thread (() => {
+                                       ready.Set ();
+                                       mre.WaitOne (1000);
+                               });
+
+                               thread.Start ();
+                               ready.WaitOne ();
+
+                               Thread.Sleep (10); // wait a bit so we enter mre.WaitOne
+
+                               DateTime end = DateTime.Now.AddMilliseconds (500);
+                               while (DateTime.Now < end) {
+                                       thread.Suspend ();
+                                       thread.Resume ();
+                               }
+
+                               Assert.IsTrue (thread.Join (1000), "#1");
+                       }
+               }
+
+               [Test]
+               public void WaitAnyWithTimeoutAndSpuriousWake ()
+               {
+                       /* This is to test that WaitEvent.WaitAny is not going to wait largely
+                        * more than its timeout. In this test, it shouldn't wait more than
+                        * 1500 milliseconds, with its timeout being 1000ms */
+
+                       using (ManualResetEvent mre1 = new ManualResetEvent (false))
+                       using (ManualResetEvent mre2 = new ManualResetEvent (false))
+                       using (ManualResetEvent ready = new ManualResetEvent (false)) {
+                               var thread = new Thread (() => {
+                                       ready.Set ();
+                                       WaitHandle.WaitAny (new [] { mre1, mre2 }, 1000);
+                               });
+
+                               thread.Start ();
+                               ready.WaitOne ();
+
+                               Thread.Sleep (10); // wait a bit so we enter WaitHandle.WaitAny ({mre1, mre2})
+
+                               DateTime end = DateTime.Now.AddMilliseconds (500);
+                               while (DateTime.Now < end) {
+                                       thread.Suspend ();
+                                       thread.Resume ();
+                               }
+
+                               Assert.IsTrue (thread.Join (1000), "#1");
+                       }
+               }
+
+               [Test]
+               public void WaitAllWithTimeoutAndSpuriousWake ()
+               {
+                       /* This is to test that WaitEvent.WaitAll is not going to wait largely
+                        * more than its timeout. In this test, it shouldn't wait more than
+                        * 1500 milliseconds, with its timeout being 1000ms */
+
+                       using (ManualResetEvent mre1 = new ManualResetEvent (false))
+                       using (ManualResetEvent mre2 = new ManualResetEvent (false))
+                       using (ManualResetEvent ready = new ManualResetEvent (false)) {
+                               var thread = new Thread (() => {
+                                       ready.Set ();
+                                       WaitHandle.WaitAll (new [] { mre1, mre2 }, 1000);
+                               });
+
+                               thread.Start ();
+                               ready.WaitOne ();
+
+                               Thread.Sleep (10); // wait a bit so we enter WaitHandle.WaitAll ({mre1, mre2})
+
+                               DateTime end = DateTime.Now.AddMilliseconds (500);
+                               while (DateTime.Now < end) {
+                                       thread.Suspend ();
+                                       thread.Resume ();
+                               }
+
+                               Assert.IsTrue (thread.Join (1000), "#1");
+                       }
+               }
        }
 }
 

diff --git a/mono/io-layer/wait.c b/mono/io-layer/wait.c
index 82a441ee3b8c71a792c3186fb6111db4be7f8a7a..a91fff80d57c0697f6431d99b75fd9327a945433 100644 (file)
--- a/mono/io-layer/wait.c
+++ b/mono/io-layer/wait.c
@@ -17,6 +17,7 @@
 #include <mono/io-layer/wapi-private.h>
 #include <mono/io-layer/io-trace.h>
 #include <mono/utils/mono-logger-internals.h>
+#include <mono/utils/mono-time.h>
 
 static gboolean own_if_signalled(gpointer handle)
 {
@@ -88,6 +89,7 @@ guint32 WaitForSingleObjectEx(gpointer handle, guint32 timeout,
        int thr_ret;
        gboolean apc_pending = FALSE;
        gpointer current_thread = wapi_get_current_thread_handle ();
+       gint64 now, end;
        
        if (current_thread == NULL) {
                SetLastError (ERROR_INVALID_HANDLE);
@@ -157,6 +159,9 @@ guint32 WaitForSingleObjectEx(gpointer handle, guint32 timeout,
                goto done;
        }
        
+       if (timeout != INFINITE)
+               end = mono_100ns_ticks () + timeout * 1000 * 10;
+
        do {
                /* Check before waiting on the condition, just in case
                 */
@@ -170,7 +175,17 @@ guint32 WaitForSingleObjectEx(gpointer handle, guint32 timeout,
                        goto done;
                }
 
-               waited = _wapi_handle_timedwait_signal_handle (handle, timeout, alertable, FALSE, &apc_pending);
+               if (timeout == INFINITE) {
+                       waited = _wapi_handle_timedwait_signal_handle (handle, INFINITE, alertable, FALSE, &apc_pending);
+               } else {
+                       now = mono_100ns_ticks ();
+                       if (end < now) {
+                               ret = WAIT_TIMEOUT;
+                               goto done;
+                       }
+
+                       waited = _wapi_handle_timedwait_signal_handle (handle, (end - now) / 10 / 1000, alertable, FALSE, &apc_pending);
+               }
 
                if(waited==0 && !apc_pending) {
                        /* Condition was signalled, so hopefully
@@ -253,6 +268,7 @@ guint32 SignalObjectAndWait(gpointer signal_handle, gpointer wait,
        int thr_ret;
        gboolean apc_pending = FALSE;
        gpointer current_thread = wapi_get_current_thread_handle ();
+       gint64 now, end;
        
        if (current_thread == NULL) {
                SetLastError (ERROR_INVALID_HANDLE);
@@ -327,6 +343,9 @@ guint32 SignalObjectAndWait(gpointer signal_handle, gpointer wait,
                goto done;
        }
 
+       if (timeout != INFINITE)
+               end = mono_100ns_ticks () + timeout * 1000 * 10;
+
        do {
                /* Check before waiting on the condition, just in case
                 */
@@ -339,7 +358,17 @@ guint32 SignalObjectAndWait(gpointer signal_handle, gpointer wait,
                        goto done;
                }
 
-               waited = _wapi_handle_timedwait_signal_handle (wait, timeout, alertable, FALSE, &apc_pending);
+               if (timeout == INFINITE) {
+                       waited = _wapi_handle_timedwait_signal_handle (wait, INFINITE, alertable, FALSE, &apc_pending);
+               } else {
+                       now = mono_100ns_ticks ();
+                       if (end < now) {
+                               ret = WAIT_TIMEOUT;
+                               goto done;
+                       }
+
+                       waited = _wapi_handle_timedwait_signal_handle (wait, (end - now) / 10 / 1000, alertable, FALSE, &apc_pending);
+               }
 
                if (waited==0 && !apc_pending) {
                        /* Condition was signalled, so hopefully
@@ -445,6 +474,7 @@ guint32 WaitForMultipleObjectsEx(guint32 numobjects, gpointer *handles,
        gboolean poll;
        gpointer sorted_handles [MAXIMUM_WAIT_OBJECTS];
        gboolean apc_pending = FALSE;
+       gint64 now, end;
        
        if (current_thread == NULL) {
                SetLastError (ERROR_INVALID_HANDLE);
@@ -528,6 +558,10 @@ guint32 WaitForMultipleObjectsEx(guint32 numobjects, gpointer *handles,
        if (timeout == 0) {
                return WAIT_TIMEOUT;
        }
+
+       if (timeout != INFINITE)
+               end = mono_100ns_ticks () + timeout * 1000 * 10;
+
        /* Have to wait for some or all handles to become signalled
         */
 
@@ -571,7 +605,16 @@ guint32 WaitForMultipleObjectsEx(guint32 numobjects, gpointer *handles,
                
                if (!done) {
                        /* Enter the wait */
-                       ret = _wapi_handle_timedwait_signal (timeout, poll, &apc_pending);
+                       if (timeout == INFINITE) {
+                               ret = _wapi_handle_timedwait_signal (INFINITE, poll, &apc_pending);
+                       } else {
+                               now = mono_100ns_ticks ();
+                               if (end < now) {
+                                       ret = WAIT_TIMEOUT;
+                               } else {
+                                       ret = _wapi_handle_timedwait_signal ((end - now) / 10 / 1000, poll, &apc_pending);
+                               }
+                       }
                } else {
                        /* No need to wait */
                        ret = 0;