Created attachment 295639 [details] [review]
gtask: Add a GTask::returned signal

This is emitted immediately after the task’s main callback, in the same
main context as that callback. It allows for multiple bits of code to
listen for completion of the GTask, which opens the door for blocking on
cancellation of a GTask and improved handling of ‘pending’ behaviour.

Created attachment 295640 [details] [review]
gtask: Add g_task_has_returned()

This can be used to query whether the task has returned, in the sense
that it has had a result set on it, and has already – or will soon –
invoke its callback function.

Review of attachment 295640 [details] [review]:

LGTM

::: gio/gtask.h
@@ +154,3 @@
 gboolean      g_task_had_error                 (GTask           *task);
+GLIB_AVAILABLE_IN_2_36
+gboolean      g_task_has_returned              (GTask           *task);

GLIB_AVAILABLE_IN_2_44

Review of attachment 295639 [details] [review]:

LGTM

previously discussed in bug 732960 (which should probably be duped to this one if this gets committed)

Comment on attachment 295639 [details] [review]
gtask: Add a GTask::returned signal

>+typedef enum {
>+	SIGNAL_RETURNED = 1,
>+} GTaskSignal;
>+#define N_SIGNALS (SIGNAL_RETURNED + 1)
>+
>+static guint signals[N_SIGNALS];

Use the same style as (most of) the rest of gio please. Anonymous enum, no "SIGNAL_" in the signal name, and a "LAST_SIGNAL" enum member rather than a separate #define.

>+   * GTask::returned:
>+   *
>+   * Emitted in the same main context as the taskâs callback, immediately after
>+   * that callback is emitted.

Given that it's tied to the invocation of the callback, not to g_task_return_*(), "returned" doesn't seem like the best name. "completed"?

>@@ -1509,8 +1507,8 @@ test_return_pointer (void)
>   g_assert_cmpint (object->ref_count, ==, 1);
> 
>   g_object_unref (task);
>-  g_assert (task == NULL);
>-  g_assert (object == NULL);
>+  g_assert (task != NULL);  /* signal emission in idle */
>+  g_assert (object != NULL);

The test case changes need to be thought through more carefully. Eg, according to the comment above it, this test is supposed to be testing that "if we don't read back the return value, the task will run its destroy_notify". But it's not actually doing that any more. (It's also no longer testing that the task doesn't get leaked in this scenario.)

There also needs to be a test of the new functionality.

Comment on attachment 295640 [details] [review]
gtask: Add g_task_has_returned()

>+  task->callback_invoked = FALSE;

you don't need to do that; the struct is initialized to all 0s, and we don't explicitly initialize the 0/FALSE/NULL members.

>+ * Checks whether @task has had g_task_return_pointer(), g_task_return_error(),
>+ * or any of the other return functions called on it. If it has, it has
>+ * completed and is guaranteed to have invoked its callback, or will invoke its
>+ * callback shortly.

So this doesn't match up with the ::returned signal then. That's weird. Why do you want post-callback semantics there, but possibly-pre-callback semantics here? (Especially odd: if the callback is invoked via an idle, then g_task_has_returned() will be TRUE when the callback is called, but if it's invoked immediately, then g_task_has_returned() will be FALSE at that point.)

You're already using basically this code somewhere, right? Can you post a link to it? Or a sample of the code? It would be useful to see what it looks like in actual use (and to compare that against the other possibility that was suggested somewhere, which is to have a counter of pending subtasks, and not invoke the callback until that reaches 0).

Created attachment 295860 [details]
Example usage code implementing blocking cancellation of multiple GTasks

Here’s some sample code. It implements a foo_task_cancel_multiple_async() function to block on completing cancellation of one or more GTasks, as briefly mentioned in bug #732960, comment #3.

(In reply to comment #7)
> >+ * Checks whether @task has had g_task_return_pointer(), g_task_return_error(),
> >+ * or any of the other return functions called on it. If it has, it has
> >+ * completed and is guaranteed to have invoked its callback, or will invoke its
> >+ * callback shortly.
> 
> So this doesn't match up with the ::returned signal then. That's weird. Why do
> you want post-callback semantics there, but possibly-pre-callback semantics
> here? (Especially odd: if the callback is invoked via an idle, then
> g_task_has_returned() will be TRUE when the callback is called, but if it's
> invoked immediately, then g_task_has_returned() will be FALSE at that point.)

I think you’re right, the semantics should match up better. In fact, I agree with all of your review comments, and will update the patches once you’ve taken a look at the sample code (attachment #295860 [details]) just in case that sparks some fresh comments.

(In reply to comment #6)
> >+   * GTask::returned:
> >+   *
> >+   * Emitted in the same main context as the taskâs callback, immediately after
> >+   * that callback is emitted.
> 
> Given that it's tied to the invocation of the callback, not to
> g_task_return_*(), "returned" doesn't seem like the best name. "completed"?

I went with ‘returned’ because I thought ‘completed’ had associations with successful completion, whereas this signal is emitted on successful and unsuccessful completion. But I think I’m wrong; ‘completed’ would be better.

(In reply to Philip Withnall from comment #9)
> Created attachment 295860 [details]
> Example usage code implementing blocking cancellation of multiple GTasks
> 
> Here’s some sample code. It implements a foo_task_cancel_multiple_async()
> function to block on completing cancellation of one or more GTasks, as
> briefly mentioned in bug #732960, comment #3.

OK, so this is something that couldn't be implemented with just the internal count of pending subtasks, since there's no way foo_task_cancel_multiple_async() could know when the tasks completed without having a signal. So, ok, this approach looks good. (Maybe we'll want the pending count as well later, but we clearly need at least the signal.)

Although, actually, it should be a property and you can connect to notify::completed, rather than being a signal. And then g_task_has_returned() would become g_task_get_completed(), which would just return the value of that property.

Created attachment 298693 [details] [review]
gtask: Add a GTask:completed property

This can be used to query whether the task has completed, in the sense
that it has had a result set on it, and has already – or will soon –
invoke its callback function.

Notifications for this property are emitted immediately after the task’s
main callback, in the same main context as that callback. This allows
for multiple bits of code to listen for completion of the GTask, which
opens the door for blocking on cancellation of the GTask and improved
handling of ‘pending’ behaviour.

Here’s an updated version with all your review comments applied, I think. It doesn’t explicitly include a new test case for the functionality, but does modify a number of the other cases to test the :completed property behaviour more explicitly. I think this is a better approach than a new test, since the property interacts tightly with the rest of GTask.

Comment on attachment 298693 [details] [review]
gtask: Add a GTask:completed property

>+  gboolean callback_invoked;

might as well call it "completed"

>@@ -1103,7 +1119,7 @@ g_task_return (GTask           *task,
>   if (type == G_TASK_RETURN_SUCCESS)
>     task->result_set = TRUE;
> 
>-  if (task->synchronous || !task->callback)
>+  if (task->synchronous)
>     return;

So :completed won't get set on synchronous tasks? That seems wrong. Though it would probably be better to do the notification from the end of g_task_run_in_thread_sync() rather than here, so that the property is notified in the same thread you invoke the task in.

>+ * g_task_get_completed:
>+ * @task: a #GTask.
>+ *
>+ * Gets the value of #GTask:completed.

It would probably be good to reiterate here that this is still %FALSE during the callback.

>+   * GTask:completed:
>+   *
>+   * Whether the task has had g_task_return_pointer(), g_task_return_error(),
>+   * or any of the other return functions called on it. If it has, it has
>+   * completed and is guaranteed to have invoked its callback.
>+   *
>+   * This property is guaranteed to change from %FALSE to %TRUE exactly once.
>+   *
>+   * The #GObject::notify signal for this change is emitted in the same main
>+   * context as the taskâs callback, immediately after that callback is invoked.

The first paragraph still sounds more like the original semantics, and should be talking only about the callback, not the return function. Except also, you should clarify "immediately after that callback is invoked (or after a callback would have been invoked, if the task has no callback)."


The test changes look good. I'm fine with getting this into 2.44 once the sync task issue is resolved.

Created attachment 298900 [details] [review]
gtask: Add a GTask:completed property

This can be used to query whether the task has completed, in the sense
that it has had a result set on it, and has already – or will soon –
invoke its callback function.

Notifications for this property are emitted immediately after the task’s
main callback, in the same main context as that callback. This allows
for multiple bits of code to listen for completion of the GTask, which
opens the door for blocking on cancellation of the GTask and improved
handling of ‘pending’ behaviour.

(In reply to Dan Winship from comment #14)
> Comment on attachment 298693 [details] [review] [review]
> gtask: Add a GTask:completed property
> 
> >+  gboolean callback_invoked;
> 
> might as well call it "completed"

Done.

> >@@ -1103,7 +1119,7 @@ g_task_return (GTask           *task,
> >   if (type == G_TASK_RETURN_SUCCESS)
> >     task->result_set = TRUE;
> > 
> >-  if (task->synchronous || !task->callback)
> >+  if (task->synchronous)
> >     return;
> 
> So :completed won't get set on synchronous tasks? That seems wrong. Though
> it would probably be better to do the notification from the end of
> g_task_run_in_thread_sync() rather than here, so that the property is
> notified in the same thread you invoke the task in.

Added to g_task_run_in_thread_sync(), and added the relevant assertions to the unit tests I missed out before. That means we’re also checking the behaviour on cancellation more now.

> >+ * g_task_get_completed:
> >+ * @task: a #GTask.
> >+ *
> >+ * Gets the value of #GTask:completed.
> 
> It would probably be good to reiterate here that this is still %FALSE during
> the callback.

Updated to:
> * Gets the value of #GTask:completed. This changes from %FALSE to %TRUE after
> * the task’s callback is invoked, and will return %FALSE if called from inside
> * the callback.

> >+   * GTask:completed:
> >+   *
> >+   * Whether the task has had g_task_return_pointer(), g_task_return_error(),
> >+   * or any of the other return functions called on it. If it has, it has
> >+   * completed and is guaranteed to have invoked its callback.
> >+   *
> >+   * This property is guaranteed to change from %FALSE to %TRUE exactly once.
> >+   *
> >+   * The #GObject::notify signal for this change is emitted in the same main
> >+   * context as the taskâs callback, immediately after that callback is invoked.
> 
> The first paragraph still sounds more like the original semantics, and
> should be talking only about the callback, not the return function. Except
> also, you should clarify "immediately after that callback is invoked (or
> after a callback would have been invoked, if the task has no callback)."

True. Updated to:
> * Whether the task has completed, meaning its callback (if set) has been
> * invoked. This can only happen after g_task_return_pointer(),
> * g_task_return_error() or one of the other return functions have been called
> * on the task.

Comment on attachment 298900 [details] [review]
gtask: Add a GTask:completed property

>+                          P_("Task completed"),
>+                          P_("Whether the task has completed yet"),

I'm not sure whether P_() is considered to break string freeze or not. You can either figure that out, and notify the i18n list if so, or else remove it for now, but remember to add it back post-branch.

Merged. Thanks for all your reviews.

(P_() does not affect the .po files, since it is not listed in the xgettext call in po/Makefile.in.in, unlike N_(), C_(), etc.)

Attachment 298900 [details] pushed as 4f1f68e - gtask: Add a GTask:completed property

*** Bug 732960 has been marked as a duplicate of this bug. ***