diff --git a/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs b/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs index 8f5692efef6b3a0d9832ad17e102846ec05e183f..2653293c95d16a7c2d7b3be2b8e2ad6e17a8144d 100644 --- a/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs +++ b/mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs @@ -409,7 +409,7 @@ namespace MonoTests.System.Threading { m.WaitOne (); }); thread1.Start (); - thread1.Join (1000); + Assert.IsTrue (thread1.Join (Timeout.Infinite), "thread1.Join"); try { m.WaitOne (); Assert.Fail ("Expected AbandonedMutexException"); @@ -421,7 +421,7 @@ namespace MonoTests.System.Threading { signalled = m.WaitOne (100); }); thread2.Start (); - thread2.Join (1000); + Assert.IsTrue (thread2.Join (Timeout.Infinite), "thread2.Join"); Assert.IsFalse (signalled); // Since this thread owns the Mutex releasing it shouldn't fail @@ -489,7 +489,7 @@ namespace MonoTests.System.Threading { m1.ReleaseMutex (); }); thread1.Start (); - thread1.Join (1000); + Assert.IsTrue (thread1.Join (Timeout.Infinite), "thread1.Join"); thread2.Start (); while (!mainProceed) { Thread.Sleep (10); @@ -502,7 +502,7 @@ namespace MonoTests.System.Threading { Assert.AreEqual (m2, e.Mutex); } finally { thread2Proceed = true; - thread2.Join (1000); + Assert.IsTrue (thread2.Join (Timeout.Infinite), "thread2.Join"); } // Current thread should own the second Mutex now @@ -511,7 +511,7 @@ namespace MonoTests.System.Threading { signalled = WaitHandle.WaitAny (new WaitHandle [] { m1, m2 }, 0); }); thread3.Start (); - thread3.Join (1000); + Assert.IsTrue (thread3.Join (Timeout.Infinite), "thread3.Join"); Assert.AreEqual (0, signalled); // Since this thread owns the second Mutex releasing it shouldn't fail @@ -540,7 +540,7 @@ namespace MonoTests.System.Threading { m1.WaitOne (); }); thread.Start (); - thread.Join (1000); + Assert.IsTrue (thread.Join (Timeout.Infinite), "thread.Join"); WaitHandle.WaitAll (new WaitHandle [] { m1, m2 }); } } diff --git a/mono/metadata/threads.c b/mono/metadata/threads.c index 6ddf1b7b84447666ff6ef6ba1a2a671daf44a0b9..379c7a7c6bdee0808347e0e3b52823de27ce56a4 100644 --- a/mono/metadata/threads.c +++ b/mono/metadata/threads.c @@ -128,9 +128,9 @@ static void mono_threads_unlock (void); static MonoCoopMutex threads_mutex; /* Controls access to the 'joinable_threads' hash table */ -#define joinable_threads_lock() mono_os_mutex_lock (&joinable_threads_mutex) -#define joinable_threads_unlock() mono_os_mutex_unlock (&joinable_threads_mutex) -static mono_mutex_t joinable_threads_mutex; +#define joinable_threads_lock() mono_coop_mutex_lock (&joinable_threads_mutex) +#define joinable_threads_unlock() mono_coop_mutex_unlock (&joinable_threads_mutex) +static MonoCoopMutex joinable_threads_mutex; /* Holds current status of static data heap */ static StaticDataInfo thread_static_info; @@ -161,10 +161,19 @@ static MonoGHashTable *threads_starting_up = NULL; static GHashTable *joinable_threads; static gint32 joinable_thread_count; +/* mono_threads_join_threads will take threads from joinable_threads list and wait for them. */ +/* When this happens, the tid is not on the list anymore so mono_thread_join assumes the thread has complete */ +/* and will return back to the caller. This could cause a race since caller of join assumes thread has completed */ +/* and on some OS it could cause errors. Keeping the tid's currently pending a native thread join call */ +/* in a separate table (only affecting callers interested in this internal join detail) and look at that table in mono_thread_join */ +/* will close this race. */ +static GHashTable *pending_native_thread_join_calls; +static MonoCoopCond pending_native_thread_join_calls_event; + static GHashTable *pending_joinable_threads; static gint32 pending_joinable_thread_count; -static mono_cond_t zero_pending_joinable_thread_event; +static MonoCoopCond zero_pending_joinable_thread_event; static void threads_add_pending_joinable_runtime_thread (MonoThreadInfo *mono_thread_info); static gboolean threads_wait_pending_joinable_threads (uint32_t timeout); @@ -3036,11 +3045,12 @@ void mono_thread_init (MonoThreadStartCB start_cb, mono_coop_mutex_init_recursive (&threads_mutex); mono_os_mutex_init_recursive(&interlocked_mutex); - mono_os_mutex_init_recursive(&joinable_threads_mutex); + mono_coop_mutex_init_recursive(&joinable_threads_mutex); mono_os_event_init (&background_change_event, FALSE); - mono_os_cond_init (&zero_pending_joinable_thread_event); + mono_coop_cond_init (&pending_native_thread_join_calls_event); + mono_coop_cond_init (&zero_pending_joinable_thread_event); mono_init_static_data_info (&thread_static_info); mono_init_static_data_info (&context_static_info); @@ -3161,7 +3171,8 @@ mono_thread_cleanup (void) mono_os_mutex_destroy (&interlocked_mutex); mono_os_mutex_destroy (&delayed_free_table_mutex); mono_os_mutex_destroy (&small_id_mutex); - mono_os_cond_destroy (&zero_pending_joinable_runtime_thread_event); + mono_coop_cond_destroy (&zero_pending_joinable_thread_event); + mono_coop_cond_destroy (&pending_native_thread_join_calls_event); mono_os_event_destroy (&background_change_event); #endif } @@ -5280,7 +5291,7 @@ threads_remove_pending_joinable_thread_nolock (gpointer tid) if (pending_joinable_threads && g_hash_table_lookup_extended (pending_joinable_threads, tid, &orig_key, &value)) { g_hash_table_remove (pending_joinable_threads, tid); if (UnlockedDecrement (&pending_joinable_thread_count) == 0) - mono_os_cond_broadcast (&zero_pending_joinable_thread_event); + mono_coop_cond_broadcast (&zero_pending_joinable_thread_event); } } @@ -5291,12 +5302,12 @@ threads_wait_pending_joinable_threads (uint32_t timeout) joinable_threads_lock (); if (timeout == MONO_INFINITE_WAIT) { while (UnlockedRead (&pending_joinable_thread_count) > 0) - mono_os_cond_wait (&zero_pending_joinable_thread_event, &joinable_threads_mutex); + mono_coop_cond_wait (&zero_pending_joinable_thread_event, &joinable_threads_mutex); } else { gint64 start = mono_msec_ticks (); gint64 elapsed = 0; while (UnlockedRead (&pending_joinable_thread_count) > 0 && elapsed < timeout) { - mono_os_cond_timedwait (&zero_pending_joinable_thread_event, &joinable_threads_mutex, timeout - (uint32_t)elapsed); + mono_coop_cond_timedwait (&zero_pending_joinable_thread_event, &joinable_threads_mutex, timeout - (uint32_t)elapsed); elapsed = mono_msec_ticks () - start; } } @@ -5344,6 +5355,39 @@ mono_threads_add_joinable_runtime_thread (gpointer thread_info) } } +static void +threads_add_pending_native_thread_join_call_nolock (gpointer tid) +{ + if (!pending_native_thread_join_calls) + pending_native_thread_join_calls = g_hash_table_new (NULL, NULL); + + gpointer orig_key; + gpointer value; + + if (!g_hash_table_lookup_extended (pending_native_thread_join_calls, tid, &orig_key, &value)) + g_hash_table_insert (pending_native_thread_join_calls, tid, tid); +} + +static void +threads_remove_pending_native_thread_join_call_nolock (gpointer tid) +{ + if (pending_native_thread_join_calls) + g_hash_table_remove (pending_native_thread_join_calls, tid); + + mono_coop_cond_broadcast (&pending_native_thread_join_calls_event); +} + +static void +threads_wait_pending_native_thread_join_call_nolock (gpointer tid) +{ + gpointer orig_key; + gpointer value; + + while (g_hash_table_lookup_extended (pending_native_thread_join_calls, tid, &orig_key, &value)) { + mono_coop_cond_wait (&pending_native_thread_join_calls_event, &joinable_threads_mutex); + } +} + /* * mono_add_joinable_thread: * @@ -5375,9 +5419,9 @@ void mono_threads_join_threads (void) { GHashTableIter iter; - gpointer key; - gpointer value; - gboolean found; + gpointer key = NULL; + gpointer value = NULL; + gboolean found = FALSE; /* Fastpath */ if (!UnlockedRead (&joinable_thread_count)) @@ -5385,6 +5429,10 @@ mono_threads_join_threads (void) while (TRUE) { joinable_threads_lock (); + if (found) { + // Previous native thread join call completed. + threads_remove_pending_native_thread_join_call_nolock (key); + } found = FALSE; if (g_hash_table_size (joinable_threads)) { g_hash_table_iter_init (&iter, joinable_threads); @@ -5392,6 +5440,9 @@ mono_threads_join_threads (void) g_hash_table_remove (joinable_threads, key); UnlockedDecrement (&joinable_thread_count); found = TRUE; + + // Add to table of tid's with pending native thread join call. + threads_add_pending_native_thread_join_call_nolock (key); } joinable_threads_unlock (); if (found) @@ -5422,13 +5473,27 @@ mono_thread_join (gpointer tid) g_hash_table_remove (joinable_threads, tid); UnlockedDecrement (&joinable_thread_count); found = TRUE; + + // Add to table of tid's with pending native join call. + threads_add_pending_native_thread_join_call_nolock (tid); + } + + if (!found) { + // Wait for any pending native thread join call not yet completed for this tid. + threads_wait_pending_native_thread_join_call_nolock (tid); } + joinable_threads_unlock (); if (!found) return; threads_native_thread_join_nolock (tid, value); + + joinable_threads_lock (); + // Native thread join call completed for this tid. + threads_remove_pending_native_thread_join_call_nolock (tid); + joinable_threads_unlock (); } void