Skip to content
Permalink
Browse files

[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
  • Loading branch information...
luhenry committed Feb 19, 2016
1 parent 1ad39fa commit 55492cbbee242e6fc844863c6360411e77ae3c52
Showing with 134 additions and 3 deletions.
  1. +88 −0 mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs
  2. +46 −3 mono/io-layer/wait.c
@@ -395,6 +395,94 @@ public void InterrupedWaitOne ()
}
}

[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");
}
}
}
}

@@ -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;

0 comments on commit 55492cb

Please sign in to comment.
You can’t perform that action at this time.