[threadpool] Fix hang on threadpool cleanup (#4330)

[mono.git] / mono / metadata / threadpool.c

diff --git a/mono/metadata/threadpool.c b/mono/metadata/threadpool.c
index cfe2892b375e949f3defab0e49046bf85e2641d0..e58a78d47e0e37e9669762067b68316268d9caf5 100644 (file)
--- a/mono/metadata/threadpool.c
+++ b/mono/metadata/threadpool.c
@@ -131,7 +131,13 @@ destroy (gpointer unused)
        mono_coop_mutex_destroy (&threadpool->threads_lock);
        mono_coop_cond_destroy (&threadpool->threads_exit_cond);
 
-       g_free (threadpool);
+       /* We cannot free the threadpool, because there is a race
+        * on shutdown where a managed thread may request a new
+        * threadpool thread, but we already destroyed the
+        * threadpool. So to avoid a use-after-free, we simply do
+        * not free the threadpool, as we won't be able to access
+        * the threadpool anyway because the ref count will be 0 */
+       // g_free (threadpool);
 }
 
 static void
@@ -181,29 +187,26 @@ cleanup (void)
 
        mono_coop_mutex_unlock (&threadpool->threads_lock);
 
+#if 0
        /* give a chance to the other threads to exit */
        mono_thread_info_yield ();
 
        mono_coop_mutex_lock (&threadpool->threads_lock);
 
        for (;;) {
-               ThreadPoolCounter counter;
-
-               counter = COUNTER_READ (threadpool);
-               if (counter._.working == 0)
+               if (threadpool->threads->len == 0)
                        break;
 
-               if (counter._.working == 1) {
-                       if (threadpool->threads->len == 1 && g_ptr_array_index (threadpool->threads, 0) == current) {
-                               /* We are waiting on ourselves */
-                               break;
-                       }
+               if (threadpool->threads->len == 1 && g_ptr_array_index (threadpool->threads, 0) == current) {
+                       /* We are waiting on ourselves */
+                       break;
                }
 
                mono_coop_cond_wait (&threadpool->threads_exit_cond, &threadpool->threads_lock);
        }
 
        mono_coop_mutex_unlock (&threadpool->threads_lock);
+#endif
 
        mono_threadpool_worker_cleanup (threadpool->worker);
 
@@ -345,6 +348,15 @@ tpdomain_get_next (ThreadPoolDomain *current)
        return tpdomain;
 }
 
+static MonoObject*
+try_invoke_perform_wait_callback (MonoObject** exc, MonoError *error)
+{
+       HANDLE_FUNCTION_ENTER ();
+       mono_error_init (error);
+       MonoObject *res = mono_runtime_try_invoke (mono_defaults.threadpool_perform_wait_callback_method, NULL, NULL, exc, error);
+       HANDLE_FUNCTION_RETURN_VAL (res);
+}
+
 static void
 worker_callback (gpointer unused)
 {
@@ -418,7 +430,7 @@ worker_callback (gpointer unused)
                if (mono_domain_set (tpdomain->domain, FALSE)) {
                        MonoObject *exc = NULL, *res;
 
-                       res = mono_runtime_try_invoke (mono_defaults.threadpool_perform_wait_callback_method, NULL, NULL, &exc, &error);
+                       res = try_invoke_perform_wait_callback (&exc, &error);
                        if (exc || !mono_error_ok(&error)) {
                                if (exc == NULL)
                                        exc = (MonoObject *) mono_error_convert_to_exception (&error);
@@ -458,16 +470,16 @@ worker_callback (gpointer unused)
 
        mono_coop_mutex_lock (&threadpool->threads_lock);
 
-       COUNTER_ATOMIC (threadpool, counter, {
-               counter._.working --;
-       });
-
        g_ptr_array_remove_fast (threadpool->threads, thread);
 
        mono_coop_cond_signal (&threadpool->threads_exit_cond);
 
        mono_coop_mutex_unlock (&threadpool->threads_lock);
 
+       COUNTER_ATOMIC (threadpool, counter, {
+               counter._.working --;
+       });
+
        mono_refcount_dec (threadpool);
 }
 
@@ -556,7 +568,7 @@ mono_threadpool_end_invoke (MonoAsyncResult *ares, MonoArray **out_args, MonoObj
                        g_assert(wait_event);
                        MonoWaitHandle *wait_handle = mono_wait_handle_new (mono_object_domain (ares), wait_event, error);
                        if (!is_ok (error)) {
-                               CloseHandle (wait_event);
+                               mono_w32event_close (wait_event);
                                return NULL;
                        }
                        MONO_OBJECT_SETREF (ares, handle, (MonoObject*) wait_handle);
@@ -792,11 +804,17 @@ ves_icall_System_Threading_ThreadPool_RequestWorkerThread (void)
        if (mono_domain_is_unloading (domain))
                return FALSE;
 
+       if (!mono_refcount_tryinc (threadpool)) {
+               /* threadpool has been destroyed, we are shutting down */
+               return FALSE;
+       }
+
        domains_lock ();
 
        /* synchronize with mono_threadpool_remove_domain_jobs */
        if (mono_domain_is_unloading (domain)) {
                domains_unlock ();
+               mono_refcount_dec (threadpool);
                return FALSE;
        }
 
@@ -806,19 +824,19 @@ ves_icall_System_Threading_ThreadPool_RequestWorkerThread (void)
        tpdomain->outstanding_request ++;
        g_assert (tpdomain->outstanding_request >= 1);
 
-       mono_refcount_inc (threadpool);
+       domains_unlock ();
 
        COUNTER_ATOMIC (threadpool, counter, {
-               if (!(counter._.starting < 32767 /* G_MAXINT16 */))
-                       g_error ("%s: counter._.starting = %d, but should be < 32767", __func__, counter._.starting);
+               if (counter._.starting == 16) {
+                       mono_refcount_dec (threadpool);
+                       return TRUE;
+               }
 
                counter._.starting ++;
        });
 
        mono_threadpool_worker_enqueue (threadpool->worker, worker_callback, NULL);
 
-       domains_unlock ();
-
        return TRUE;
 }