From 5b126081576f73159449954f65ab7eeff11145f2 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 17 Jun 2020 18:06:20 +0100 Subject: [PATCH 1/5] tests: Fix intermittent failure in GCancellableSource test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It seems that allowing the GCancellable to be finalised in either the main thread or the worker thread sometimes leads to crashes when running on CI. I cannot reproduce these crashes locally, and various analyses with memcheck, drd and helgrind have failed to give any clues. Fix this for this particular test case by deferring destruction of the `GCancellable` instances until after the worker thread has joined. That’s OK because this test is specifically checking a race between `g_cancellable_cancel()` and disposal of a `GCancellableSource`. The underlying bug remains unfixed, though, and I can only hope that we eventually find a reliable way of reproducing it so it can be analysed and fixed. Signed-off-by: Philip Withnall --- gio/tests/cancellable.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c index e53ae6e7e..8d211857f 100644 --- a/gio/tests/cancellable.c +++ b/gio/tests/cancellable.c @@ -270,6 +270,7 @@ test_cancellable_source_threaded_dispose (void) ThreadedDisposeData data; GThread *thread = NULL; guint i; + GPtrArray *cancellables_pending_unref = g_ptr_array_new_with_free_func (g_object_unref); g_test_summary ("Test a thread race between disposing of a GCancellableSource " "(in one thread) and cancelling the GCancellable it refers " @@ -309,7 +310,18 @@ test_cancellable_source_threaded_dispose (void) /* Race with disposal of the cancellable source. */ g_cancellable_cancel (cancellable); - g_object_unref (cancellable); + /* This thread can’t drop its reference to the #GCancellable here, as it + * might not be the final reference (depending on how the race is + * resolved: #GCancellableSource holds a strong ref on the #GCancellable), + * and at this point we can’t guarantee to support disposing of a + * #GCancellable in a different thread from where it’s created, especially + * when signal handlers are connected to it. + * + * So this is a workaround for a disposal-in-another-thread bug for + * #GCancellable, but there’s no hope of debugging and resolving it with + * this test setup, and the bug is orthogonal to what’s being tested here + * (a race between #GCancellable and #GCancellableSource). */ + g_ptr_array_add (cancellables_pending_unref, g_steal_pointer (&cancellable)); } /* Indicate that the test has finished. Can’t use %NULL as #GAsyncQueue @@ -322,6 +334,8 @@ test_cancellable_source_threaded_dispose (void) g_async_queue_unref (data.cancellable_source_queue); g_mutex_clear (&data.mutex); g_cond_clear (&data.cond); + + g_ptr_array_unref (cancellables_pending_unref); } int From 5571aaa1c518ae4ff50602395ae17861bf50e68e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 17 Jun 2020 18:16:45 +0100 Subject: [PATCH 2/5] tests: Use g_test_message() rather than g_printerr() in cancellable test This makes sure the message gets formatted correctly and sent to the right log file. Signed-off-by: Philip Withnall --- gio/tests/cancellable.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c index 8d211857f..a12829ecd 100644 --- a/gio/tests/cancellable.c +++ b/gio/tests/cancellable.c @@ -54,12 +54,12 @@ mock_operation_thread (GTask *task, if (g_cancellable_is_cancelled (cancellable)) break; if (g_test_verbose ()) - g_printerr ("THRD: %u iteration %u\n", data->iterations_requested, i); + g_test_message ("THRD: %u iteration %u", data->iterations_requested, i); g_usleep (WAIT_ITERATION * 1000); } if (g_test_verbose ()) - g_printerr ("THRD: %u stopped at %u\n", data->iterations_requested, i); + g_test_message ("THRD: %u stopped at %u", data->iterations_requested, i); data->iterations_done = i; g_task_return_boolean (task, TRUE); @@ -84,8 +84,8 @@ mock_operation_timeout (gpointer user_data) if (done) { if (g_test_verbose ()) - g_printerr ("LOOP: %u stopped at %u\n", data->iterations_requested,\ - data->iterations_done); + g_test_message ("LOOP: %u stopped at %u", + data->iterations_requested, data->iterations_done); g_task_return_boolean (task, TRUE); return FALSE; /* don't call timeout again */ } @@ -93,8 +93,8 @@ mock_operation_timeout (gpointer user_data) { data->iterations_done++; if (g_test_verbose ()) - g_printerr ("LOOP: %u iteration %u\n", data->iterations_requested, - data->iterations_done); + g_test_message ("LOOP: %u iteration %u", + data->iterations_requested, data->iterations_done); return TRUE; /* call timeout */ } } @@ -118,14 +118,14 @@ mock_operation_async (guint wait_iterations, { g_task_run_in_thread (task, mock_operation_thread); if (g_test_verbose ()) - g_printerr ("THRD: %d started\n", wait_iterations); + g_test_message ("THRD: %d started", wait_iterations); } else { g_timeout_add_full (G_PRIORITY_DEFAULT, WAIT_ITERATION, mock_operation_timeout, g_object_ref (task), g_object_unref); if (g_test_verbose ()) - g_printerr ("LOOP: %d started\n", wait_iterations); + g_test_message ("LOOP: %d started", wait_iterations); } g_object_unref (task); @@ -210,7 +210,7 @@ test_cancel_multiple_concurrent (void) g_main_loop_run (loop); g_assert_cmpint (num_async_operations, ==, 45); if (g_test_verbose ()) - g_printerr ("CANCEL: %d operations\n", num_async_operations); + g_test_message ("CANCEL: %d operations", num_async_operations); g_cancellable_cancel (cancellable); g_assert_true (g_cancellable_is_cancelled (cancellable)); From a956b096afa2df2b1f44d908ee03abe93a7a2692 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 17 Jun 2020 18:30:56 +0100 Subject: [PATCH 3/5] tests: Use atomics to access counter shared between threads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This should fix some sporadic test failures in this test, although I can’t be sure as I was unable to reproduce the original failure. Signed-off-by: Philip Withnall Closes: #1764 --- gio/tests/cancellable.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c index a12829ecd..97546132c 100644 --- a/gio/tests/cancellable.c +++ b/gio/tests/cancellable.c @@ -29,8 +29,8 @@ static gint num_async_operations = 0; typedef struct { - guint iterations_requested; - guint iterations_done; + guint iterations_requested; /* construct-only */ + guint iterations_done; /* (atomic) */ } MockOperationData; static void @@ -60,7 +60,7 @@ mock_operation_thread (GTask *task, if (g_test_verbose ()) g_test_message ("THRD: %u stopped at %u", data->iterations_requested, i); - data->iterations_done = i; + g_atomic_int_add (&data->iterations_done, i); g_task_return_boolean (task, TRUE); } @@ -71,11 +71,13 @@ mock_operation_timeout (gpointer user_data) GTask *task; MockOperationData *data; gboolean done = FALSE; + guint iterations_done; task = G_TASK (user_data); data = g_task_get_task_data (task); + iterations_done = g_atomic_int_get (&data->iterations_done); - if (data->iterations_done >= data->iterations_requested) + if (iterations_done >= data->iterations_requested) done = TRUE; if (g_cancellable_is_cancelled (g_task_get_cancellable (task))) @@ -85,16 +87,16 @@ mock_operation_timeout (gpointer user_data) { if (g_test_verbose ()) g_test_message ("LOOP: %u stopped at %u", - data->iterations_requested, data->iterations_done); + data->iterations_requested, iterations_done); g_task_return_boolean (task, TRUE); return FALSE; /* don't call timeout again */ } else { - data->iterations_done++; + g_atomic_int_inc (&data->iterations_done); if (g_test_verbose ()) g_test_message ("LOOP: %u iteration %u", - data->iterations_requested, data->iterations_done); + data->iterations_requested, iterations_done + 1); return TRUE; /* call timeout */ } } @@ -147,7 +149,7 @@ mock_operation_finish (GAsyncResult *result, data = g_task_get_task_data (task); g_task_propagate_boolean (task, error); - return data->iterations_done; + return g_atomic_int_get (&data->iterations_done); } GMainLoop *loop; From 5e49d53cd5a8684ad4cdafcd189269931cb80483 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 17 Jun 2020 18:31:45 +0100 Subject: [PATCH 4/5] tests: Use G_SOURCE_CONTINUE/G_SOURCE_REMOVE instead of booleans This makes no functional difference, but clarifies the code a little. Signed-off-by: Philip Withnall --- gio/tests/cancellable.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c index 97546132c..62a10702e 100644 --- a/gio/tests/cancellable.c +++ b/gio/tests/cancellable.c @@ -89,7 +89,7 @@ mock_operation_timeout (gpointer user_data) g_test_message ("LOOP: %u stopped at %u", data->iterations_requested, iterations_done); g_task_return_boolean (task, TRUE); - return FALSE; /* don't call timeout again */ + return G_SOURCE_REMOVE; } else { @@ -97,7 +97,7 @@ mock_operation_timeout (gpointer user_data) if (g_test_verbose ()) g_test_message ("LOOP: %u iteration %u", data->iterations_requested, iterations_done + 1); - return TRUE; /* call timeout */ + return G_SOURCE_CONTINUE; } } From ee3216b31b098a543e32ec3444d69605d07d0eae Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 17 Jun 2020 18:32:28 +0100 Subject: [PATCH 5/5] tests: Iterate the GMainContext directly rather than using a GMainLoop This speeds up the `cancellable` test a little by stopping waiting for the threads to start up as soon as they have started, rather than after an arbitrary timeout. Signed-off-by: Philip Withnall Helps: #1764 --- gio/tests/cancellable.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/gio/tests/cancellable.c b/gio/tests/cancellable.c index 62a10702e..d6b52c1ea 100644 --- a/gio/tests/cancellable.c +++ b/gio/tests/cancellable.c @@ -152,8 +152,6 @@ mock_operation_finish (GAsyncResult *result, return g_atomic_int_get (&data->iterations_done); } -GMainLoop *loop; - static void on_mock_operation_ready (GObject *source, GAsyncResult *result, @@ -171,17 +169,7 @@ on_mock_operation_ready (GObject *source, g_assert_cmpint (iterations_requested, >, iterations_done); num_async_operations--; - - if (!num_async_operations) - g_main_loop_quit (loop); -} - -static gboolean -on_main_loop_timeout_quit (gpointer user_data) -{ - GMainLoop *loop = user_data; - g_main_loop_quit (loop); - return FALSE; + g_main_context_wakeup (NULL); } static void @@ -197,7 +185,6 @@ test_cancel_multiple_concurrent (void) } cancellable = g_cancellable_new (); - loop = g_main_loop_new (NULL, FALSE); for (i = 0; i < 45; i++) { @@ -207,21 +194,22 @@ test_cancel_multiple_concurrent (void) num_async_operations++; } - /* Wait for two iterations, to give threads a chance to start up */ - g_timeout_add (WAIT_ITERATION * 2, on_main_loop_timeout_quit, loop); - g_main_loop_run (loop); - g_assert_cmpint (num_async_operations, ==, 45); + /* Wait for the threads to start up */ + while (num_async_operations != 45) + g_main_context_iteration (NULL, TRUE); + g_assert_cmpint (num_async_operations, ==, 45);\ + if (g_test_verbose ()) g_test_message ("CANCEL: %d operations", num_async_operations); g_cancellable_cancel (cancellable); g_assert_true (g_cancellable_is_cancelled (cancellable)); /* Wait for all operations to be cancelled */ - g_main_loop_run (loop); + while (num_async_operations != 0) + g_main_context_iteration (NULL, TRUE); g_assert_cmpint (num_async_operations, ==, 0); g_object_unref (cancellable); - g_main_loop_unref (loop); } static void