[threads] Inline thread_cleanup into mono_thread_detach_internal (#4119)
authorLudovic Henry <ludovic@xamarin.com>
Fri, 9 Dec 2016 16:09:59 +0000 (11:09 -0500)
committerGitHub <noreply@github.com>
Fri, 9 Dec 2016 16:09:59 +0000 (11:09 -0500)
mono/metadata/threads.c

index 6721ba5c23fe263f8e45850aa4e82e79b906b826..953a2315008f047a7dced9b39f53a9d0f218a3f2 100644 (file)
@@ -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);