From a275ee66796ab0d6d95ed8647f2170be9b136951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 11 Jul 2022 18:45:36 +0200 Subject: [PATCH] gthreadpool: Update unused_threads while we still own the pool lock As per the rationale explained in the previous commit, we could end up having the unused_threads value not to be conformant to what g_thread_pool_get_num_threads() returns, because an about-to-be-unused thread might not be counted yet as such, while the pool threads number has been already decreased. To avoid such scenario, and to make sure that when all the pool's threads are stopped, they're unmarked as unused, let's increase the unused_threads value earlier, while we still own the pool lock so that it will always include the pool that is not used anymore, but not yet queued. As per this we can update the test, not to repeat the stop-unused call as now we're sure that when the pool has no threads anymore, the unused threads value is also updated accordingly. Also adding a tests with multiple pools. --- glib/gthreadpool.c | 10 +++--- glib/tests/thread-pool-slow.c | 63 +++++++++++++++++++++++++++++------ 2 files changed, 58 insertions(+), 15 deletions(-) diff --git a/glib/gthreadpool.c b/glib/gthreadpool.c index bcbb9e2d3..0d656f072 100644 --- a/glib/gthreadpool.c +++ b/glib/gthreadpool.c @@ -167,8 +167,6 @@ g_thread_pool_wait_for_new_pool (void) local_max_idle_time = g_atomic_int_get (&max_idle_time); last_wakeup_thread_serial = g_atomic_int_get (&wakeup_thread_serial); - g_atomic_int_inc (&unused_threads); - do { if ((guint) g_atomic_int_get (&unused_threads) >= local_max_unused_threads) @@ -237,8 +235,6 @@ g_thread_pool_wait_for_new_pool (void) } while (pool == wakeup_thread_marker); - g_atomic_int_add (&unused_threads, -1); - return pool; } @@ -405,12 +401,16 @@ g_thread_pool_thread_proxy (gpointer data) } } + g_atomic_int_inc (&unused_threads); g_async_queue_unlock (pool->queue); if (free_pool) g_thread_pool_free_internal (pool); - if ((pool = g_thread_pool_wait_for_new_pool ()) == NULL) + pool = g_thread_pool_wait_for_new_pool (); + g_atomic_int_add (&unused_threads, -1); + + if (pool == NULL) break; g_async_queue_lock (pool->queue); diff --git a/glib/tests/thread-pool-slow.c b/glib/tests/thread-pool-slow.c index a7d3039ec..ae16426be 100644 --- a/glib/tests/thread-pool-slow.c +++ b/glib/tests/thread-pool-slow.c @@ -80,18 +80,13 @@ test_thread_stop_unused (void) g_usleep (100); g_assert_cmpuint (g_thread_pool_get_num_threads (pool), ==, 0); + g_assert_cmpuint (g_thread_pool_get_num_unused_threads (), >, 0); /* Wait for threads to die. */ - do { - /* We may need to repeat this in case we tried to stop unused threads - * while some thread was still active, and not yet marked as non-used, - * despite what g_thread_pool_get_num_threads() tells us. - * And if this happens the thread will be kept in the unused queue - * indefinitely, so we need to stop it again, until we're really done. - */ - g_thread_pool_stop_unused_threads (); + g_thread_pool_stop_unused_threads (); + + while (g_thread_pool_get_num_unused_threads () != 0) g_usleep (100); - } while (g_thread_pool_get_num_unused_threads () != 0); g_assert_cmpuint (g_thread_pool_get_num_unused_threads (), ==, 0); @@ -103,6 +98,51 @@ test_thread_stop_unused (void) g_thread_pool_free (pool, FALSE, TRUE); } +static void +test_thread_stop_unused_multiple (void) +{ + GThreadPool *pools[10]; + guint i, j; + const guint limit = 10; + gboolean all_stopped; + + /* Spawn a few threads. */ + g_thread_pool_set_max_unused_threads (-1); + + for (i = 0; i < G_N_ELEMENTS (pools); i++) + { + pools[i] = g_thread_pool_new ((GFunc) g_usleep, NULL, -1, FALSE, NULL); + + for (j = 0; j < limit; j++) + g_thread_pool_push (pools[i], GUINT_TO_POINTER (100), NULL); + } + + all_stopped = FALSE; + while (!all_stopped) + { + all_stopped = TRUE; + for (i = 0; i < G_N_ELEMENTS (pools); i++) + all_stopped &= (g_thread_pool_get_num_threads (pools[i]) == 0); + } + + for (i = 0; i < G_N_ELEMENTS (pools); i++) + { + g_assert_cmpuint (g_thread_pool_get_num_threads (pools[i]), ==, 0); + g_assert_cmpuint (g_thread_pool_get_num_unused_threads (), >, 0); + } + + /* Wait for threads to die. */ + g_thread_pool_stop_unused_threads (); + + while (g_thread_pool_get_num_unused_threads () != 0) + g_usleep (100); + + g_assert_cmpuint (g_thread_pool_get_num_unused_threads (), ==, 0); + + for (i = 0; i < G_N_ELEMENTS (pools); i++) + g_thread_pool_free (pools[i], FALSE, TRUE); +} + static void test_thread_pools_entry_func (gpointer data, gpointer user_data) { @@ -344,6 +384,9 @@ test_check_start_and_stop (gpointer user_data) test_thread_stop_unused (); break; case 7: + test_thread_stop_unused_multiple (); + break; + case 8: test_thread_idle_time (); break; default: @@ -373,7 +416,7 @@ test_check_start_and_stop (gpointer user_data) G_UNLOCK (thread_counter_sort); } - if (test_number == 7) { + if (test_number == 8) { guint idle; idle = g_thread_pool_get_num_unused_threads ();