From fcf7d1f425615aa5bf7f02c56247a4764359549d Mon Sep 17 00:00:00 2001 From: Ludovic Henry Date: Fri, 9 Dec 2016 11:09:59 -0500 Subject: [PATCH] [threads] Inline thread_cleanup into mono_thread_detach_internal (#4119) --- mono/metadata/threads.c | 289 +++++++++++++++++++--------------------- 1 file changed, 140 insertions(+), 149 deletions(-) diff --git a/mono/metadata/threads.c b/mono/metadata/threads.c index 6721ba5c23f..953a2315008 100644 --- a/mono/metadata/threads.c +++ b/mono/metadata/threads.c @@ -318,147 +318,6 @@ is_threadabort_exception (MonoClass *klass) return klass == mono_defaults.threadabortexception_class; } -/* - * NOTE: this function can be called also for threads different from the current one: - * make sure no code called from it will ever assume it is run on the thread that is - * getting cleaned up. - */ -static void thread_cleanup (MonoInternalThread *thread) -{ - gboolean ret; - - g_assert (thread != NULL); - - if (thread->abort_state_handle) { - mono_gchandle_free (thread->abort_state_handle); - thread->abort_state_handle = 0; - } - thread->abort_exc = NULL; - thread->current_appcontext = NULL; - - /* - * This is necessary because otherwise we might have - * cross-domain references which will not get cleaned up when - * the target domain is unloaded. - */ - if (thread->cached_culture_info) { - int i; - for (i = 0; i < NUM_CACHED_CULTURES * 2; ++i) - mono_array_set (thread->cached_culture_info, MonoObject*, i, NULL); - } - - /* - * thread->synch_cs can be NULL if this was called after - * ves_icall_System_Threading_InternalThread_Thread_free_internal. - * This can happen only during shutdown. - * The shutting_down flag is not always set, so we can't assert on it. - */ - if (thread->synch_cs) - LOCK_THREAD (thread); - - thread->state |= ThreadState_Stopped; - thread->state &= ~ThreadState_Background; - - if (thread->synch_cs) - UNLOCK_THREAD (thread); - - /* - An interruption request has leaked to cleanup. Adjust the global counter. - - This can happen is the abort source thread finds the abortee (this) thread - in unmanaged code. If this thread never trips back to managed code or check - the local flag it will be left set and positively unbalance the global counter. - - Leaving the counter unbalanced will cause a performance degradation since all threads - will now keep checking their local flags all the time. - */ - if (InterlockedExchange (&thread->interruption_requested, 0)) - InterlockedDecrement (&thread_interruption_requested); - - mono_threads_lock (); - - if (!threads) { - ret = FALSE; - } else if (mono_g_hash_table_lookup (threads, (gpointer)thread->tid) != thread) { - /* We have to check whether the thread object for the - * tid is still the same in the table because the - * thread might have been destroyed and the tid reused - * in the meantime, in which case the tid would be in - * the table, but with another thread object. - */ - ret = FALSE; - } else { - mono_g_hash_table_remove (threads, (gpointer)thread->tid); - ret = TRUE; - } - - mono_threads_unlock (); - - /* Don't close the handle here, wait for the object finalizer - * to do it. Otherwise, the following race condition applies: - * - * 1) Thread exits (and thread_cleanup() closes the handle) - * - * 2) Some other handle is reassigned the same slot - * - * 3) Another thread tries to join the first thread, and - * blocks waiting for the reassigned handle to be signalled - * (which might never happen). This is possible, because the - * thread calling Join() still has a reference to the first - * thread's object. - */ - - /* if the thread is not in the hash it has been removed already */ - if (!ret) { - if (thread == mono_thread_internal_current ()) { - mono_domain_unset (); - mono_memory_barrier (); - } - if (mono_thread_cleanup_fn) - mono_thread_cleanup_fn (thread_get_tid (thread)); - return; - } - mono_release_type_locks (thread); - - /* Can happen when we attach the profiler helper thread in order to heapshot. */ - if (!mono_thread_info_lookup (MONO_UINT_TO_NATIVE_THREAD_ID (thread->tid))->tools_thread) - mono_profiler_thread_end (thread->tid); - - mono_hazard_pointer_clear (mono_hazard_pointer_get (), 1); - - if (thread == mono_thread_internal_current ()) { - /* - * This will signal async signal handlers that the thread has exited. - * The profiler callback needs this to be set, so it cannot be done earlier. - */ - mono_domain_unset (); - mono_memory_barrier (); - } - - if (thread == mono_thread_internal_current ()) - mono_thread_pop_appdomain_ref (); - - thread->cached_culture_info = NULL; - - mono_free_static_data (thread->static_data); - thread->static_data = NULL; - ref_stack_destroy (thread->appdomain_refs); - thread->appdomain_refs = NULL; - - if (mono_thread_cleanup_fn) - mono_thread_cleanup_fn (thread_get_tid (thread)); - - if (mono_gc_is_moving ()) { - MONO_GC_UNREGISTER_ROOT (thread->thread_pinning_ref); - thread->thread_pinning_ref = NULL; - } - - g_assert (thread->suspended); - mono_os_event_destroy (thread->suspended); - g_free (thread->suspended); - thread->suspended = NULL; -} - /* * A special static data offset (guint32) consists of 3 parts: * @@ -876,14 +735,15 @@ static guint32 WINAPI start_wrapper_internal(StartInfo *start_info, gsize *stack /* If the thread calls ExitThread at all, this remaining code * will not be executed, but the main thread will eventually - * call thread_cleanup() on this thread's behalf. + * call mono_thread_detach_internal() on this thread's behalf. */ THREAD_DEBUG (g_message ("%s: (%"G_GSIZE_FORMAT") Start wrapper terminating", __func__, mono_native_thread_id_get ())); /* Do any cleanup needed for apartment state. This - * cannot be done in thread_cleanup since thread_cleanup could be - * called for a thread other than the current thread. + * cannot be done in mono_thread_detach_internal since + * mono_thread_detach_internal could be called for a + * thread other than the current thread. * mono_thread_cleanup_apartment_state cleans up apartment * for the current thead */ mono_thread_cleanup_apartment_state (); @@ -1127,7 +987,9 @@ mono_thread_attach_full (MonoDomain *domain, gboolean force_attach) void mono_thread_detach_internal (MonoInternalThread *thread) { - g_return_if_fail (thread != NULL); + gboolean removed; + + g_assert (thread != NULL); THREAD_DEBUG (g_message ("%s: mono_thread_detach for %p (%"G_GSIZE_FORMAT")", __func__, thread, (gsize)thread->tid)); @@ -1135,8 +997,136 @@ mono_thread_detach_internal (MonoInternalThread *thread) mono_w32mutex_abandon (); #endif - thread_cleanup (thread); + if (thread->abort_state_handle) { + mono_gchandle_free (thread->abort_state_handle); + thread->abort_state_handle = 0; + } + + thread->abort_exc = NULL; + thread->current_appcontext = NULL; + + /* + * This is necessary because otherwise we might have + * cross-domain references which will not get cleaned up when + * the target domain is unloaded. + */ + if (thread->cached_culture_info) { + int i; + for (i = 0; i < NUM_CACHED_CULTURES * 2; ++i) + mono_array_set (thread->cached_culture_info, MonoObject*, i, NULL); + } + + /* + * thread->synch_cs can be NULL if this was called after + * ves_icall_System_Threading_InternalThread_Thread_free_internal. + * This can happen only during shutdown. + * The shutting_down flag is not always set, so we can't assert on it. + */ + if (thread->synch_cs) + LOCK_THREAD (thread); + + thread->state |= ThreadState_Stopped; + thread->state &= ~ThreadState_Background; + + if (thread->synch_cs) + UNLOCK_THREAD (thread); + + /* + An interruption request has leaked to cleanup. Adjust the global counter. + + This can happen is the abort source thread finds the abortee (this) thread + in unmanaged code. If this thread never trips back to managed code or check + the local flag it will be left set and positively unbalance the global counter. + + Leaving the counter unbalanced will cause a performance degradation since all threads + will now keep checking their local flags all the time. + */ + if (InterlockedExchange (&thread->interruption_requested, 0) != 0) + InterlockedDecrement (&thread_interruption_requested); + + mono_threads_lock (); + + if (!threads) { + removed = FALSE; + } else if (mono_g_hash_table_lookup (threads, (gpointer)thread->tid) != thread) { + /* We have to check whether the thread object for the + * tid is still the same in the table because the + * thread might have been destroyed and the tid reused + * in the meantime, in which case the tid would be in + * the table, but with another thread object. + */ + removed = FALSE; + } else { + mono_g_hash_table_remove (threads, (gpointer)thread->tid); + removed = TRUE; + } + + mono_threads_unlock (); + + /* Don't close the handle here, wait for the object finalizer + * to do it. Otherwise, the following race condition applies: + * + * 1) Thread exits (and mono_thread_detach_internal() closes the handle) + * + * 2) Some other handle is reassigned the same slot + * + * 3) Another thread tries to join the first thread, and + * blocks waiting for the reassigned handle to be signalled + * (which might never happen). This is possible, because the + * thread calling Join() still has a reference to the first + * thread's object. + */ + + /* if the thread is not in the hash it has been removed already */ + if (!removed) { + mono_domain_unset (); + mono_memory_barrier (); + + if (mono_thread_cleanup_fn) + mono_thread_cleanup_fn (thread_get_tid (thread)); + + goto done; + } + + mono_release_type_locks (thread); + + /* Can happen when we attach the profiler helper thread in order to heapshot. */ + if (!mono_thread_info_lookup (MONO_UINT_TO_NATIVE_THREAD_ID (thread->tid))->tools_thread) + mono_profiler_thread_end (thread->tid); + mono_hazard_pointer_clear (mono_hazard_pointer_get (), 1); + + /* + * This will signal async signal handlers that the thread has exited. + * The profiler callback needs this to be set, so it cannot be done earlier. + */ + mono_domain_unset (); + mono_memory_barrier (); + + if (thread == mono_thread_internal_current ()) + mono_thread_pop_appdomain_ref (); + + thread->cached_culture_info = NULL; + + mono_free_static_data (thread->static_data); + thread->static_data = NULL; + ref_stack_destroy (thread->appdomain_refs); + thread->appdomain_refs = NULL; + + if (mono_thread_cleanup_fn) + mono_thread_cleanup_fn (thread_get_tid (thread)); + + if (mono_gc_is_moving ()) { + MONO_GC_UNREGISTER_ROOT (thread->thread_pinning_ref); + thread->thread_pinning_ref = NULL; + } + + g_assert (thread->suspended); + mono_os_event_destroy (thread->suspended); + g_free (thread->suspended); + thread->suspended = NULL; + +done: SET_CURRENT_OBJECT (NULL); mono_domain_unset (); @@ -1273,9 +1263,10 @@ ves_icall_System_Threading_InternalThread_Thread_free_internal (MonoInternalThre THREAD_DEBUG (g_message ("%s: Closing thread %p, handle %p", __func__, this, this_obj->handle)); /* - * Since threads keep a reference to their thread object while running, by the time this function is called, - * the thread has already exited/detached, i.e. thread_cleanup () has ran. The exception is during shutdown, - * when thread_cleanup () can be called after this. + * Since threads keep a reference to their thread object while running, by + * the time this function is called, the thread has already exited/detached, + * i.e. mono_thread_detach_internal () has ran. The exception is during + * shutdown, when mono_thread_detach_internal () can be called after this. */ if (this_obj->handle) { mono_threads_close_thread_handle (this_obj->handle); -- 2.25.1