From 9c412c0b91987abfed25f20486e9ef992cb5a1af Mon Sep 17 00:00:00 2001 From: Ludovic Henry Date: Mon, 5 Jun 2017 18:07:40 -0400 Subject: [PATCH] [w32handle] Fix deadlock on SignalAndWait (#4973) Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=57069 --- .../Test/System.Threading/WaitHandleTest.cs | 13 ++++++ mono/metadata/w32event-unix.c | 21 ++++++++-- mono/metadata/w32handle.c | 2 +- mono/metadata/w32handle.h | 2 +- mono/metadata/w32mutex-unix.c | 41 ++++++++++++++++--- mono/metadata/w32semaphore-unix.c | 26 ++++++++++-- 6 files changed, 90 insertions(+), 15 deletions(-) diff --git a/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs b/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs index 593edcf65c6..7ceec51affc 100644 --- a/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs +++ b/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs @@ -628,6 +628,19 @@ namespace MonoTests.System.Threading { } } #endif // MONO_FEATURE_THREAD_SUSPEND_RESUME + + [Test] + public static void SignalAndWait() + { + using (var eventToSignal = new AutoResetEvent (false)) + using (var eventToWait = new AutoResetEvent (false)) + { + eventToWait.Set (); + + Assert.IsTrue (WaitHandle.SignalAndWait (eventToSignal, eventToWait), "#1"); + Assert.IsTrue (eventToSignal.WaitOne (), "#2"); + } + } } } diff --git a/mono/metadata/w32event-unix.c b/mono/metadata/w32event-unix.c index d99023e9e34..37ec577a6b0 100644 --- a/mono/metadata/w32event-unix.c +++ b/mono/metadata/w32event-unix.c @@ -36,6 +36,19 @@ struct MonoW32HandleNamedEvent { MonoW32HandleNamespace sharedns; }; +static void event_handle_signal (gpointer handle, MonoW32HandleType type, MonoW32HandleEvent *event_handle) +{ + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s: signalling %s handle %p", + __func__, mono_w32handle_get_typename (type), handle); + + if (!event_handle->manual) { + event_handle->set_count = 1; + mono_w32handle_set_signal_state (handle, TRUE, FALSE); + } else { + mono_w32handle_set_signal_state (handle, TRUE, TRUE); + } +} + static gboolean event_handle_own (gpointer handle, MonoW32HandleType type, gboolean *abandoned) { MonoW32HandleEvent *event_handle; @@ -64,9 +77,9 @@ static gboolean event_handle_own (gpointer handle, MonoW32HandleType type, gbool return TRUE; } -static void event_signal(gpointer handle) +static void event_signal(gpointer handle, gpointer handle_specific) { - ves_icall_System_Threading_Events_SetEvent_internal (handle); + event_handle_signal (handle, MONO_W32HANDLE_EVENT, (MonoW32HandleEvent*) handle_specific); } static gboolean event_own (gpointer handle, gboolean *abandoned) @@ -74,9 +87,9 @@ static gboolean event_own (gpointer handle, gboolean *abandoned) return event_handle_own (handle, MONO_W32HANDLE_EVENT, abandoned); } -static void namedevent_signal (gpointer handle) +static void namedevent_signal (gpointer handle, gpointer handle_specific) { - ves_icall_System_Threading_Events_SetEvent_internal (handle); + event_handle_signal (handle, MONO_W32HANDLE_NAMEDEVENT, (MonoW32HandleEvent*) handle_specific); } /* NB, always called with the shared handle lock held */ diff --git a/mono/metadata/w32handle.c b/mono/metadata/w32handle.c index 6500ba7fcf0..f6862a9a7db 100644 --- a/mono/metadata/w32handle.c +++ b/mono/metadata/w32handle.c @@ -809,7 +809,7 @@ mono_w32handle_ops_signal (gpointer handle) type = handle_data->type; if (handle_ops[type] != NULL && handle_ops[type]->signal != NULL) { - handle_ops[type]->signal (handle); + handle_ops[type]->signal (handle, handle_data->specific); } } diff --git a/mono/metadata/w32handle.h b/mono/metadata/w32handle.h index 741dd083235..029f24dad82 100644 --- a/mono/metadata/w32handle.h +++ b/mono/metadata/w32handle.h @@ -53,7 +53,7 @@ typedef struct void (*close)(gpointer handle, gpointer data); /* mono_w32handle_signal_and_wait */ - void (*signal)(gpointer signal); + void (*signal)(gpointer signal, gpointer data); /* Called by mono_w32handle_wait_one and mono_w32handle_wait_multiple, * with the handle locked (shared handles aren't locked.) diff --git a/mono/metadata/w32mutex-unix.c b/mono/metadata/w32mutex-unix.c index 7c339ceda08..189b808ba22 100644 --- a/mono/metadata/w32mutex-unix.c +++ b/mono/metadata/w32mutex-unix.c @@ -64,6 +64,38 @@ thread_disown_mutex (MonoInternalThread *internal, gpointer handle) mono_w32handle_unref (handle); } +static void +mutex_handle_signal (gpointer handle, MonoW32HandleType type, MonoW32HandleMutex *mutex_handle) +{ + pthread_t tid; + + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s: signalling %s handle %p, tid: %p recursion: %d", + __func__, mono_w32handle_get_typename (type), handle, (gpointer) mutex_handle->tid, mutex_handle->recursion); + + tid = pthread_self (); + + if (mutex_handle->abandoned) { + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s: %s handle %p is abandoned", + __func__, mono_w32handle_get_typename (type), handle); + } else if (!pthread_equal (mutex_handle->tid, tid)) { + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s: we don't own %s handle %p (owned by %ld, me %ld)", + __func__, mono_w32handle_get_typename (type), handle, (long)mutex_handle->tid, (long)tid); + } else { + /* OK, we own this mutex */ + mutex_handle->recursion--; + + if (mutex_handle->recursion == 0) { + thread_disown_mutex (mono_thread_internal_current (), handle); + + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s: unlocking %s handle %p, tid: %p recusion : %d", + __func__, mono_w32handle_get_typename (type), handle, (gpointer) mutex_handle->tid, mutex_handle->recursion); + + mutex_handle->tid = 0; + mono_w32handle_set_signal_state (handle, TRUE, FALSE); + } + } +} + static gboolean mutex_handle_own (gpointer handle, MonoW32HandleType type, gboolean *abandoned) { @@ -123,9 +155,9 @@ mutex_handle_is_owned (gpointer handle, MonoW32HandleType type) } } -static void mutex_signal(gpointer handle) +static void mutex_signal(gpointer handle, gpointer handle_specific) { - ves_icall_System_Threading_Mutex_ReleaseMutex_internal (handle); + mutex_handle_signal (handle, MONO_W32HANDLE_MUTEX, (MonoW32HandleMutex*) handle_specific); } static gboolean mutex_own (gpointer handle, gboolean *abandoned) @@ -135,13 +167,12 @@ static gboolean mutex_own (gpointer handle, gboolean *abandoned) static gboolean mutex_is_owned (gpointer handle) { - return mutex_handle_is_owned (handle, MONO_W32HANDLE_MUTEX); } -static void namedmutex_signal (gpointer handle) +static void namedmutex_signal (gpointer handle, gpointer handle_specific) { - ves_icall_System_Threading_Mutex_ReleaseMutex_internal (handle); + mutex_handle_signal (handle, MONO_W32HANDLE_NAMEDMUTEX, (MonoW32HandleMutex*) handle_specific); } /* NB, always called with the shared handle lock held */ diff --git a/mono/metadata/w32semaphore-unix.c b/mono/metadata/w32semaphore-unix.c index 22336c2fd45..0db4d5946fd 100644 --- a/mono/metadata/w32semaphore-unix.c +++ b/mono/metadata/w32semaphore-unix.c @@ -27,6 +27,24 @@ struct MonoW32HandleNamedSemaphore { MonoW32HandleNamespace sharedns; }; +static void sem_handle_signal (gpointer handle, MonoW32HandleType type, MonoW32HandleSemaphore *sem_handle) +{ + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s: signalling %s handle %p", + __func__, mono_w32handle_get_typename (type), handle); + + /* No idea why max is signed, but thats the spec :-( */ + if (sem_handle->val + 1 > (guint32)sem_handle->max) { + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s: %s handle %p val %d count %d max %d, max value would be exceeded", + __func__, mono_w32handle_get_typename (type), handle, sem_handle->val, 1, sem_handle->max); + } else { + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s: %s handle %p val %d count %d max %d", + __func__, mono_w32handle_get_typename (type), handle, sem_handle->val, 1, sem_handle->max); + + sem_handle->val += 1; + mono_w32handle_set_signal_state (handle, TRUE, TRUE); + } +} + static gboolean sem_handle_own (gpointer handle, MonoW32HandleType type, gboolean *abandoned) { MonoW32HandleSemaphore *sem_handle; @@ -50,9 +68,9 @@ static gboolean sem_handle_own (gpointer handle, MonoW32HandleType type, gboolea return TRUE; } -static void sema_signal(gpointer handle) +static void sema_signal(gpointer handle, gpointer handle_specific) { - ves_icall_System_Threading_Semaphore_ReleaseSemaphore_internal(handle, 1, NULL); + sem_handle_signal (handle, MONO_W32HANDLE_SEM, (MonoW32HandleSemaphore*) handle_specific); } static gboolean sema_own (gpointer handle, gboolean *abandoned) @@ -60,9 +78,9 @@ static gboolean sema_own (gpointer handle, gboolean *abandoned) return sem_handle_own (handle, MONO_W32HANDLE_SEM, abandoned); } -static void namedsema_signal (gpointer handle) +static void namedsema_signal (gpointer handle, gpointer handle_specific) { - ves_icall_System_Threading_Semaphore_ReleaseSemaphore_internal (handle, 1, NULL); + sem_handle_signal (handle, MONO_W32HANDLE_NAMEDSEM, (MonoW32HandleSemaphore*) handle_specific); } /* NB, always called with the shared handle lock held */ -- 2.25.1