[threads] At shutdown, don't wait for native threads that aren't calling Mono

src/mono/mono/metadata/threads-types.h

@@ -294,7 +294,8 @@ MONO_PROFILER_API MonoInternalThread *mono_thread_internal_current (void);
 MonoInternalThreadHandle
 mono_thread_internal_current_handle (void);
 
-void mono_thread_internal_abort (MonoInternalThread *thread, gboolean appdomain_unload);
+gboolean
+mono_thread_internal_abort (MonoInternalThread *thread, gboolean appdomain_unload);
 void mono_thread_internal_suspend_for_shutdown (MonoInternalThread *thread);
 
 gboolean mono_thread_internal_has_appdomain_ref (MonoInternalThread *thread, MonoDomain *domain);

src/mono/mono/metadata/threads.c

@@ -212,7 +212,7 @@ static void mono_free_static_data (gpointer* static_data);
 static void mono_init_static_data_info (StaticDataInfo *static_data);
 static guint32 mono_alloc_static_data_slot (StaticDataInfo *static_data, guint32 size, guint32 align);
 static gboolean mono_thread_resume (MonoInternalThread* thread);
-static void async_abort_internal (MonoInternalThread *thread, gboolean install_async_abort);
+static gboolean async_abort_internal (MonoInternalThread *thread, gboolean install_async_abort);
 static void self_abort_internal (MonoError *error);
 static void async_suspend_internal (MonoInternalThread *thread, gboolean interrupt);
 static void self_suspend_internal (void);
@@ -1866,7 +1866,7 @@ ves_icall_System_Threading_Thread_Thread_internal (MonoThreadObjectHandle thread
 
 	internal->state &= ~ThreadState_Unstarted;
 
-	THREAD_DEBUG (g_message ("%s: Started thread ID %" G_GSIZE_FORMAT " (handle %p)", __func__, tid, thread));
+	THREAD_DEBUG (g_message ("%s: Started thread ID %" G_GSIZE_FORMAT " (handle %p)", __func__, (gsize)internal->tid, internal));
 
 	UNLOCK_THREAD (internal);
 	return TRUE;
@@ -2898,15 +2898,16 @@ ves_icall_System_Threading_Thread_Abort (MonoInternalThreadHandle thread_handle,
  * mono_thread_internal_abort:
  * Request thread \p thread to be aborted.
  * \p thread MUST NOT be the current thread.
+ * \returns true if the request was successful
  */
-void
+gboolean
 mono_thread_internal_abort (MonoInternalThread *thread, gboolean appdomain_unload)
 {
 	g_assert (thread != mono_thread_internal_current ());
 
 	if (!request_thread_abort (thread, NULL, appdomain_unload))
-		return;
-	async_abort_internal (thread, TRUE);
+		return FALSE;
+	return async_abort_internal (thread, TRUE);
 }
 
 #ifndef ENABLE_NETCORE
@@ -3776,12 +3777,20 @@ abort_threads (gpointer key, gpointer value, gpointer user)
 	if ((thread->flags & MONO_THREAD_FLAG_DONT_MANAGE))
 		return;
 
-	wait->handles[wait->num] = mono_threads_open_thread_handle (thread->handle);
-	wait->threads[wait->num] = thread;
-	wait->num++;
-
+	MonoThreadHandle *handle = mono_threads_open_thread_handle (thread->handle);
 	THREAD_DEBUG (g_print ("%s: Aborting id: %" G_GSIZE_FORMAT "\n", __func__, (gsize)thread->tid));
-	mono_thread_internal_abort (thread, FALSE);
+	if (!mono_thread_internal_abort (thread, FALSE)) {
+		g_warning ("%s: Failed aborting id: %p, mono_thread_manage will ignore it\n", __func__, (void*)(intptr_t)(gsize)thread->tid);
+		/* close the handle, we're not going to wait for the thread to be aborted */
+		mono_threads_close_thread_handle (handle);
+	} else {
+		/* commit to waiting for the thread to be aborted */
+		wait->handles[wait->num] = handle;
+		wait->threads[wait->num] = thread;
+		wait->num++;
+	}
+
+
 }
 
 /** 
@@ -5615,8 +5624,11 @@ mono_thread_info_get_last_managed (MonoThreadInfo *info)
 }
 
 typedef struct {
+	/* inputs */
 	MonoInternalThread *thread;
 	gboolean install_async_abort;
+	/* outputs */
+	gboolean thread_will_abort;
 	MonoThreadInfoInterruptToken *interrupt_token;
 } AbortThreadData;
 
@@ -5629,6 +5641,8 @@ async_abort_critical (MonoThreadInfo *info, gpointer ud)
 	gboolean protected_wrapper;
 	gboolean running_managed;
 
+	data->thread_will_abort = TRUE;
+
 	if (mono_get_eh_callbacks ()->mono_install_handler_block_guard (mono_thread_info_get_suspend_state (info)))
 		return MonoResumeThread;
 
@@ -5656,25 +5670,41 @@ async_abort_critical (MonoThreadInfo *info, gpointer ud)
 		 */
 		data->interrupt_token = mono_thread_info_prepare_interrupt (info);
 
+		if (!ji && !info->runtime_thread) {
+			/* Under full cooperative suspend, if a thread is in GC Safe mode (blocking
+			 * state), mono_thread_info_safe_suspend_and_run will treat the thread as
+			 * suspended and run async_abort_critical.  If the thread has no managed
+			 * frames, it is some native thread that may not call Mono again anymore, so
+			 * don't wait for it to abort.
+			 */
+			data->thread_will_abort = FALSE;
+		}
 		return MonoResumeThread;
 	}
 }
 
-static void
+static gboolean
 async_abort_internal (MonoInternalThread *thread, gboolean install_async_abort)
 {
 	AbortThreadData data;
 
 	g_assert (thread != mono_thread_internal_current ());
 
 	data.thread = thread;
+	data.thread_will_abort = FALSE;
 	data.install_async_abort = install_async_abort;
 	data.interrupt_token = NULL;
 
 	mono_thread_info_safe_suspend_and_run (thread_get_tid (thread), TRUE, async_abort_critical, &data);
 	if (data.interrupt_token)
 		mono_thread_info_finish_interrupt (data.interrupt_token);
 	/*FIXME we need to wait for interruption to complete -- figure out how much into interruption we should wait for here*/
+
+	/* If the thread was not suspended (async_abort_critical did not run), or the thread is
+	 * "suspended" (BLOCKING) under full coop, and the thread was running native code without
+	 * any managed callers, we can't be sure that it will aknowledge the abort request and
+	 * actually abort. */
+	return data.thread_will_abort;
 }
 
 static void

src/mono/mono/tests/libtest.c

@@ -8375,19 +8375,39 @@ invoke_foreign_thread (void* user_data)
 	destroy_invoke_names (names);
 	return NULL;
 }
+
+static void*
+invoke_foreign_delegate (void *user_data)
+{
+	VoidVoidCallback del = (VoidVoidCallback)user_data;
+	for (int i = 0; i < 5; ++i) {
+		del ();
+		sleep (2);
+	}
+	return NULL;
+}
+
 #endif
 
 
 LIBTEST_API mono_bool STDCALL
-mono_test_attach_invoke_foreign_thread (const char *assm_name, const char *name_space, const char *name, const char *meth_name)
+mono_test_attach_invoke_foreign_thread (const char *assm_name, const char *name_space, const char *name, const char *meth_name, VoidVoidCallback del)
 {
 #ifndef HOST_WIN32
-	struct invoke_names *names = make_invoke_names (assm_name, name_space, name, meth_name);
-	pthread_t t;
-	int res = pthread_create (&t, NULL, invoke_foreign_thread, (void*)names);
-	g_assert (res == 0);
-	pthread_join (t, NULL);
-	return 0;
+	if (!del) {
+		struct invoke_names *names = make_invoke_names (assm_name, name_space, name, meth_name);
+		pthread_t t;
+		int res = pthread_create (&t, NULL, invoke_foreign_thread, (void*)names);
+		g_assert (res == 0);
+		pthread_join (t, NULL);
+		return 0;
+	} else {
+		pthread_t t;
+		int res = pthread_create (&t, NULL, invoke_foreign_delegate, del);
+		g_assert (res == 0);
+		pthread_join (t, NULL);
+		return 0;
+	}
 #else
 	// TODO: Win32 version of this test
 	return 1;
@@ -8396,6 +8416,8 @@ mono_test_attach_invoke_foreign_thread (const char *assm_name, const char *name_
 
 #ifndef HOST_WIN32
 struct names_and_mutex {
+	/* if del is NULL, use names, otherwise just call del */
+	VoidVoidCallback del;
 	struct invoke_names *names;
         /* mutex to coordinate test and foreign thread */
         pthread_mutex_t coord_mutex;
@@ -8410,7 +8432,11 @@ invoke_block_foreign_thread (void *user_data)
 	// This thread calls into the runtime and then blocks. It should not
 	// prevent the runtime from shutting down.
 	struct names_and_mutex *nm = (struct names_and_mutex *)user_data;
-	test_invoke_by_name (nm->names);
+	if (!nm->del) {
+		test_invoke_by_name (nm->names);
+	} else {
+		nm->del ();
+	}
         pthread_mutex_lock (&nm->coord_mutex);
         /* signal the test thread that we called the runtime */
         pthread_cond_signal (&nm->coord_cond);
@@ -8422,26 +8448,31 @@ invoke_block_foreign_thread (void *user_data)
 #endif
 
 LIBTEST_API mono_bool STDCALL
-mono_test_attach_invoke_block_foreign_thread (const char *assm_name, const char *name_space, const char *name, const char *meth_name)
+mono_test_attach_invoke_block_foreign_thread (const char *assm_name, const char *name_space, const char *name, const char *meth_name, VoidVoidCallback del)
 {
 #ifndef HOST_WIN32
-	struct invoke_names *names = make_invoke_names (assm_name, name_space, name, meth_name);
 	struct names_and_mutex *nm = malloc (sizeof (struct names_and_mutex));
-	nm->names = names;
-        pthread_mutex_init (&nm->coord_mutex, NULL);
-        pthread_cond_init (&nm->coord_cond, NULL);
+	nm->del = del;
+	if (!del) {
+		struct invoke_names *names = make_invoke_names (assm_name, name_space, name, meth_name);
+		nm->names = names;
+	} else {
+		nm->names = NULL;
+	}
+	pthread_mutex_init (&nm->coord_mutex, NULL);
+	pthread_cond_init (&nm->coord_cond, NULL);
 	pthread_mutex_init (&nm->deadlock_mutex, NULL);
 
 	pthread_mutex_lock (&nm->deadlock_mutex); // lock the mutex and never unlock it.
 	pthread_t t;
 	int res = pthread_create (&t, NULL, invoke_block_foreign_thread, (void*)nm);
 	g_assert (res == 0);
-        /* wait for the foreign thread to finish calling the runtime before
-         * detaching it and returning
-         */
-        pthread_mutex_lock (&nm->coord_mutex);
-        pthread_cond_wait (&nm->coord_cond, &nm->coord_mutex);
-        pthread_mutex_unlock (&nm->coord_mutex);
+	/* wait for the foreign thread to finish calling the runtime before
+	 * detaching it and returning
+	 */
+	pthread_mutex_lock (&nm->coord_mutex);
+	pthread_cond_wait (&nm->coord_cond, &nm->coord_mutex);
+	pthread_mutex_unlock (&nm->coord_mutex);
 	pthread_detach (t);
 	return 0;
 #else

src/mono/mono/tests/pinvoke-detach-1.cs

@@ -28,29 +28,55 @@ private static void MethodInvokedFromNative ()
 	}
 
 	[DllImport ("libtest", EntryPoint="mono_test_attach_invoke_foreign_thread")]
-	public static extern bool mono_test_attach_invoke_foreign_thread (string assm_name, string name_space, string class_name, string method_name);
+	public static extern bool mono_test_attach_invoke_foreign_thread (string assm_name, string name_space, string class_name, string method_name, VoidVoidDelegate del);
 
 	public static int test_0_attach_invoke_foreign_thread ()
 	{
 		was_called = 0;
-		bool skipped = mono_test_attach_invoke_foreign_thread (typeof (Tests).Assembly.Location, "", "Tests", "MethodInvokedFromNative");
+		bool skipped = mono_test_attach_invoke_foreign_thread (typeof (Tests).Assembly.Location, "", "Tests", "MethodInvokedFromNative", null);
+		GC.Collect (); // should not hang waiting for the foreign thread
 		return skipped || was_called == 5 ? 0 : 1;
 	}
 
+	static int was_called_del;
+
+	[MonoPInvokeCallback (typeof (VoidVoidDelegate))]
+	private static void MethodInvokedFromNative_del ()
+	{
+		was_called_del++;
+	}
+
+	public static int test_0_attach_invoke_foreign_thread_delegate ()
+	{
+		var del = new VoidVoidDelegate (MethodInvokedFromNative_del);
+		was_called_del = 0;
+		bool skipped = mono_test_attach_invoke_foreign_thread (null, null, null, null, del);
+		GC.Collect (); // should not hang waiting for the foreign thread
+		return skipped || was_called_del == 5 ? 0 : 1;
+	}
+
 	[MonoPInvokeCallback (typeof (VoidVoidDelegate))]
 	private static void MethodInvokedFromNative2 ()
 	{
 	}
 
 	[DllImport ("libtest", EntryPoint="mono_test_attach_invoke_block_foreign_thread")]
-	public static extern bool mono_test_attach_invoke_block_foreign_thread (string assm_name, string name_space, string class_name, string method_name);
+	public static extern bool mono_test_attach_invoke_block_foreign_thread (string assm_name, string name_space, string class_name, string method_name, VoidVoidDelegate del);
 
 	public static int test_0_attach_invoke_block_foreign_thread ()
 	{
-		bool skipped = mono_test_attach_invoke_block_foreign_thread (typeof (Tests).Assembly.Location, "", "Tests", "MethodInvokedFromNative2");
+		bool skipped = mono_test_attach_invoke_block_foreign_thread (typeof (Tests).Assembly.Location, "", "Tests", "MethodInvokedFromNative2", null);
 		GC.Collect (); // should not hang waiting for the foreign thread
 		return 0; // really we succeed if the app can shut down without hanging
 	}
 
+	// This one fails because we haven't fully detached, so shutdown is waiting for the thread
+	public static int test_0_attach_invoke_block_foreign_thread_delegate ()
+	{
+		var del = new VoidVoidDelegate (MethodInvokedFromNative2);
+		bool skipped = mono_test_attach_invoke_block_foreign_thread (null, null, null, null, del);
+		GC.Collect (); // should not hang waiting for the foreign thread
+		return 0; // really we succeed if the app can shut down without hanging
+	}
 
 }