Merge branch '1525-task-result' into 'master'

gtask: Check an error hasn’t been returned when calling g_task_return*()

Closes #1525

See merge request GNOME/glib!368

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 check_cancellable : 1;
-  gboolean blocking_other_task;
+  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,8 +579,6 @@ struct _GTask {
     gboolean boolean;
   } result;
   GDestroyNotify result_destroy;
-  gboolean had_error;
-  gboolean result_set;
 };
 
 #define G_TASK_IS_THREADED(task) ((task)->task_func != NULL)
@@ -1176,6 +1180,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 +1603,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;
@@ -1631,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;
@@ -1654,7 +1661,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;
 
@@ -1687,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;
@@ -1709,7 +1716,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;
 
@@ -1742,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;
@@ -1772,7 +1779,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 +1840,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))
     {

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 */
@@ -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();