Created attachment 209642 [details] [review]
GSimpleAsyncResult: support reliable cancellation

Add a function g_simple_async_result_set_check_cancellable() to provide
a GCancellable that is checked for being cancelled during the call to
g_simple_async_result_propagate_error().

This gives asynchronous operation implementations an easy way to
provide reliable cancellation of those operations -- even in the case
that a positive result has occured and is pending dispatch at the time
the operation is cancelled.

Created attachment 209644 [details] [review]
GSimpleAsyncResult: support reliable cancellation

Tweak from last patch:

  - use g_cancellable_set_error_if_cancelled()
  - don't leak check_cancellable
  - put the new function in the header

Review of attachment 209644 [details] [review]:

Missing a test case...may be hard to write, but it'd be really great to have.

You might explicitly call out dbusmenu in the commit message as a real-world user affected by this.

::: gio/gsimpleasyncresult.c
@@ +271,3 @@
 
+  if (simple->check_cancellable)
+    g_object_unref (simple->check_cancellable);

Could use g_clear_object()

@@ +309,3 @@
  *
+ * The common convention is to create the #GSimpleAsyncResult in the
+ * function that starts the asynchronous operation and use this function

How about s/this function/the function/

Because "this function" in docs most often means "the function this documentation refers to"...

@@ +1069,3 @@
+ * is checked at the time of g_simple_async_result_propagate_error() If
+ * it is cancelled, these functions will return an "Operation was
+ * cancelled" error.

Might spell out %G_IO_ERROR_CANCELLED.

Created attachment 209648 [details] [review]
GSimpleAsyncResult: support reliable cancellation

Updated as per comments.

Good call on the that/this thing.  Even when I read it a second time (15
minutes after writing it) I wasn't entirely sure what I meant.

I normally only use g_clear_object() if I want the value to be NULL at
the end (ie: not really necessary in finalize()).

I'll mention dbusmenu in the log message of the following patches that
fix GDBus to use this new function.

Created attachment 209649 [details] [review]
GDBus: make use of reliable async cancellation

Call g_simple_async_result_set_check_cancellable() after all
GSimpleAsyncResult creation in order to take advantage of the new
reliable cancellation feature.

The guarantee of reliable cancellation fixes a bug in dbusmenu (which
was already assuming that cancellation was reliable).  See this bug:
https://bugs.launchpad.net/ubuntu/+source/libdbusmenu/+bug/953562

+++ b/docs/reference/gio/gio-sections.txt
@@ -1160,6 +1160,7 @@ GCancellable
 g_cancellable_new
 g_cancellable_is_cancelled
 g_cancellable_set_error_if_cancelled
+g_cancellable_set_error_if_cancelled

that seems buggy?

Created attachment 209707 [details] [review]
GSimpleAsyncResult: support reliable cancellation

Fixed.  Thanks Seb.

Review of attachment 209648 [details] [review]:

Looks good to me, except the gio-sections thing.

::: docs/reference/gio/gio-sections.txt
@@ +1161,3 @@
 g_cancellable_is_cancelled
 g_cancellable_set_error_if_cancelled
+g_cancellable_set_error_if_cancelled

This seems weird.

Comment on attachment 209707 [details] [review]
GSimpleAsyncResult: support reliable cancellation

Attachment 209707 [details] pushed as 4804094 - GSimpleAsyncResult: support reliable cancellation

Will wait for David to review the second patch.

(In reply to comment #9)
> Will wait for David to review the second patch.

Will do that tomorrow ... (btw, only saw this bug in passing because it was referenced in a commit message; because of time constraints / other work assignments, I currently only read GLib commits, not GLib bugs - unless filed specifically against the gdbus component)

Review of attachment 209649 [details] [review]:

Hey, I'm thinking this will allow us to do away with the ugly "cancellable" qdata hack in g_dbus_connection_send_message_with_reply_unlocked() and g_dbus_connection_send_message_with_reply_finish(), yes? Apart from that it looks fine assuming the test suite still passes etc etc

I just did the simple/obvious thing.  The tests are passing.

If you want to take advantage of the feature in more complicated ways, please do so. :)

(In reply to comment #12)
> I just did the simple/obvious thing.  The tests are passing.
> 
> If you want to take advantage of the feature in more complicated ways, please
> do so. :)

OK, just tested and works fine with nuking the cancellable ('make check' passes) so I committed your patch with this on top

http://git.gnome.org/browse/glib/commit/?id=f025c9c4f4803dd09dba12ca8f35692a0ea8050a

Guess we can close this bug now. Thanks!