From 03232bd6a60a6f8436755be862c2aca2f2e72bdb Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 1 Oct 2018 21:44:01 +0100 Subject: [PATCH 1/4] tests: Fix location of an unref in the GTask tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This method drops the last reference *it* owns to the GTask, but then continues to call methods on the GTask. This wasn’t resulting in failures because a ref in another thread kept the GTask alive, but that’s quite dodgy. Signed-off-by: Philip Withnall --- gio/tests/task.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/tests/task.c b/gio/tests/task.c index 934262e40..c66aa29a3 100644 --- a/gio/tests/task.c +++ b/gio/tests/task.c @@ -1689,7 +1689,6 @@ test_return_on_cancel_atomic (void) g_task_set_task_data (task, (gpointer)&state, NULL); g_task_run_in_thread (task, return_on_cancel_atomic_thread); - g_object_unref (task); g_assert_cmpint (state, ==, 0); @@ -1724,6 +1723,7 @@ test_return_on_cancel_atomic (void) g_object_unref (cancellable); g_mutex_unlock (&roca_mutex_1); g_mutex_unlock (&roca_mutex_2); + g_object_unref (task); } /* test_return_pointer: memory management of pointer returns */ From bea37709353533e4abb1c652a655f483e6f9aeac Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 21 Mar 2018 14:47:52 +0000 Subject: [PATCH 2/4] =?UTF-8?q?gtask:=20Check=20an=20error=20hasn=E2=80=99?= =?UTF-8?q?t=20been=20returned=20when=20calling=20g=5Ftask=5Freturn*()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These functions already check to see if a successful result has already been returned; expand them to also check to see if an error has been returned. We can’t just check GTask.result_set, as that’s actually an indicator for whether the GTask.result member is filled — when g_task_propagate_*() is called, it’s cleared again. We need a new state bit. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/1525 --- gio/gtask.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/gio/gtask.c b/gio/gtask.c index 4087543e6..a31bd613a 100644 --- a/gio/gtask.c +++ b/gio/gtask.c @@ -575,6 +575,7 @@ struct _GTask { GDestroyNotify result_destroy; gboolean had_error; gboolean result_set; + gboolean ever_returned; }; #define G_TASK_IS_THREADED(task) ((task)->task_func != NULL) @@ -1176,6 +1177,9 @@ g_task_return (GTask *task, { GSource *source; + if (type != G_TASK_RETURN_FROM_THREAD) + task->ever_returned = TRUE; + if (type == G_TASK_RETURN_SUCCESS) task->result_set = TRUE; @@ -1596,7 +1600,7 @@ g_task_return_pointer (GTask *task, GDestroyNotify result_destroy) { g_return_if_fail (G_IS_TASK (task)); - g_return_if_fail (task->result_set == FALSE); + g_return_if_fail (!task->ever_returned); task->result.pointer = result; task->result_destroy = result_destroy; @@ -1654,7 +1658,7 @@ g_task_return_int (GTask *task, gssize result) { g_return_if_fail (G_IS_TASK (task)); - g_return_if_fail (task->result_set == FALSE); + g_return_if_fail (!task->ever_returned); task->result.size = result; @@ -1709,7 +1713,7 @@ g_task_return_boolean (GTask *task, gboolean result) { g_return_if_fail (G_IS_TASK (task)); - g_return_if_fail (task->result_set == FALSE); + g_return_if_fail (!task->ever_returned); task->result.boolean = result; @@ -1772,7 +1776,7 @@ g_task_return_error (GTask *task, GError *error) { g_return_if_fail (G_IS_TASK (task)); - g_return_if_fail (task->result_set == FALSE); + g_return_if_fail (!task->ever_returned); g_return_if_fail (error != NULL); task->error = error; @@ -1833,7 +1837,7 @@ g_task_return_error_if_cancelled (GTask *task) GError *error = NULL; g_return_val_if_fail (G_IS_TASK (task), FALSE); - g_return_val_if_fail (task->result_set == FALSE, FALSE); + g_return_val_if_fail (!task->ever_returned, FALSE); if (g_cancellable_set_error_if_cancelled (task->cancellable, &error)) { From f0cecba19966e1dfcb8f21438a29c4c9f28d3aec Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 1 Oct 2018 21:45:04 +0100 Subject: [PATCH 3/4] tests: Add return ordering tests for GTask These are based on an example test program provided by Will Thompson in issue #1525. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/1525 --- gio/tests/task.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/gio/tests/task.c b/gio/tests/task.c index c66aa29a3..7f698306b 100644 --- a/gio/tests/task.c +++ b/gio/tests/task.c @@ -1987,6 +1987,136 @@ test_legacy_error (void) g_assert (simple == NULL); } +/* Various helper functions for the return tests below. */ +static void +task_complete_cb (GObject *source, + GAsyncResult *result, + gpointer user_data) +{ + GTask *task = G_TASK (result); + guint *calls = user_data; + + g_assert_cmpint (++*calls, <=, 1); + + /* Propagate the result, so it’s removed from the task’s internal state. */ + g_task_propagate_boolean (task, NULL); +} + +static void +return_twice (GTask *task) +{ + gboolean error_first = GPOINTER_TO_UINT (g_task_get_task_data (task)); + + if (error_first) + { + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_UNKNOWN, "oh no"); + g_task_return_boolean (task, TRUE); + } + else + { + g_task_return_boolean (task, TRUE); + g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_UNKNOWN, "oh no"); + } +} + +static gboolean +idle_cb (gpointer user_data) +{ + GTask *task = user_data; + return_twice (task); + g_object_unref (task); + + return G_SOURCE_REMOVE; +} + +static void +test_return_permutation (gboolean error_first, + gboolean return_in_idle) +{ + guint calls = 0; + GTask *task = NULL; + + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/issues/1525"); + + task = g_task_new (NULL, NULL, task_complete_cb, &calls); + g_task_set_task_data (task, GUINT_TO_POINTER (error_first), NULL); + + if (return_in_idle) + g_idle_add (idle_cb, g_object_ref (task)); + else + return_twice (task); + + while (calls == 0) + g_main_context_iteration (NULL, TRUE); + + g_assert_cmpint (calls, ==, 1); + + g_object_unref (task); +} + +/* Test that calling g_task_return_boolean() after g_task_return_error(), when + * returning in an idle callback, correctly results in a critical warning. */ +static void +test_return_in_idle_error_first (void) +{ + if (g_test_subprocess ()) + { + test_return_permutation (TRUE, TRUE); + return; + } + + g_test_trap_subprocess (NULL, 0, 0); + g_test_trap_assert_failed (); + g_test_trap_assert_stderr ("*CRITICAL*assertion '!task->ever_returned' failed*"); +} + +/* Test that calling g_task_return_error() after g_task_return_boolean(), when + * returning in an idle callback, correctly results in a critical warning. */ +static void +test_return_in_idle_value_first (void) +{ + if (g_test_subprocess ()) + { + test_return_permutation (FALSE, TRUE); + return; + } + + g_test_trap_subprocess (NULL, 0, 0); + g_test_trap_assert_failed (); + g_test_trap_assert_stderr ("*CRITICAL*assertion '!task->ever_returned' failed*"); +} + +/* Test that calling g_task_return_boolean() after g_task_return_error(), when + * returning synchronously, correctly results in a critical warning. */ +static void +test_return_error_first (void) +{ + if (g_test_subprocess ()) + { + test_return_permutation (TRUE, FALSE); + return; + } + + g_test_trap_subprocess (NULL, 0, 0); + g_test_trap_assert_failed (); + g_test_trap_assert_stderr ("*CRITICAL*assertion '!task->ever_returned' failed*"); +} + +/* Test that calling g_task_return_error() after g_task_return_boolean(), when + * returning synchronously, correctly results in a critical warning. */ +static void +test_return_value_first (void) +{ + if (g_test_subprocess ()) + { + test_return_permutation (FALSE, FALSE); + return; + } + + g_test_trap_subprocess (NULL, 0, 0); + g_test_trap_assert_failed (); + g_test_trap_assert_stderr ("*CRITICAL*assertion '!task->ever_returned' failed*"); +} int main (int argc, char **argv) @@ -1994,6 +2124,7 @@ main (int argc, char **argv) int ret; g_test_init (&argc, &argv, NULL); + g_test_bug_base (""); loop = g_main_loop_new (NULL, FALSE); main_thread = g_thread_self (); @@ -2021,6 +2152,10 @@ main (int argc, char **argv) g_test_add_func ("/gtask/return-pointer", test_return_pointer); g_test_add_func ("/gtask/object-keepalive", test_object_keepalive); g_test_add_func ("/gtask/legacy-error", test_legacy_error); + g_test_add_func ("/gtask/return/in-idle/error-first", test_return_in_idle_error_first); + g_test_add_func ("/gtask/return/in-idle/value-first", test_return_in_idle_value_first); + g_test_add_func ("/gtask/return/error-first", test_return_error_first); + g_test_add_func ("/gtask/return/value-first", test_return_value_first); ret = g_test_run(); From 290bb0dd1b5169806e7de2a17ad22a3432661d31 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 1 Oct 2018 21:51:18 +0100 Subject: [PATCH 4/4] gtask: Compress GTask struct using a bitfield MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are a lot of gbooleans in the private GTask struct, which seems a bit wasteful. Use a bitfield to compress the struct a bit. This reduces the size of the struct from 216 bytes to 168 bytes on my 64-bit machine. One of the fields needs to remain separate, since it’s used from a TRACE() macro which calls typeof() on it. Signed-off-by: Philip Withnall --- gio/gtask.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/gio/gtask.c b/gio/gtask.c index a31bd613a..a40bc01b4 100644 --- a/gio/gtask.c +++ b/gio/gtask.c @@ -551,20 +551,26 @@ struct _GTask { gint64 creation_time; gint priority; GCancellable *cancellable; - gboolean check_cancellable; GAsyncReadyCallback callback; gpointer callback_data; - gboolean completed; GTaskThreadFunc task_func; GMutex lock; GCond cond; - gboolean return_on_cancel; + + /* This can’t be in the bit field because we access it from TRACE(). */ gboolean thread_cancelled; - gboolean synchronous; - gboolean thread_complete; - gboolean blocking_other_task; + + gboolean check_cancellable : 1; + gboolean completed : 1; + gboolean return_on_cancel : 1; + gboolean synchronous : 1; + gboolean thread_complete : 1; + gboolean blocking_other_task : 1; + gboolean had_error : 1; + gboolean result_set : 1; + gboolean ever_returned : 1; GError *error; union { @@ -573,9 +579,6 @@ struct _GTask { gboolean boolean; } result; GDestroyNotify result_destroy; - gboolean had_error; - gboolean result_set; - gboolean ever_returned; }; #define G_TASK_IS_THREADED(task) ((task)->task_func != NULL) @@ -1635,7 +1638,7 @@ g_task_propagate_pointer (GTask *task, if (g_task_propagate_error (task, error)) return NULL; - g_return_val_if_fail (task->result_set == TRUE, NULL); + g_return_val_if_fail (task->result_set, NULL); task->result_destroy = NULL; task->result_set = FALSE; @@ -1691,7 +1694,7 @@ g_task_propagate_int (GTask *task, if (g_task_propagate_error (task, error)) return -1; - g_return_val_if_fail (task->result_set == TRUE, -1); + g_return_val_if_fail (task->result_set, -1); task->result_set = FALSE; return task->result.size; @@ -1746,7 +1749,7 @@ g_task_propagate_boolean (GTask *task, if (g_task_propagate_error (task, error)) return FALSE; - g_return_val_if_fail (task->result_set == TRUE, FALSE); + g_return_val_if_fail (task->result_set, FALSE); task->result_set = FALSE; return task->result.boolean;