Technically we should link to https://developer.gnome.org/gdata/unstable/gdata-overview.html#cancellable-support from every relevant documentation comment too.

I don't think sprinkling g_task_set_check_cancellable (..., FALSE) is the right way to solve this. I will summarise my objections [1, 2, etc.] here so that they don't get lost under hundreds of unrelated comments on an unrelated bug.

--

It is a documented requirement [3] that GAsyncResult based API will always throw G_IO_ERROR_CANCELLED if the GCancellable was triggered. Not doing so can lead to memory errors. Imagine an instance which does some asynchronous operations internally, and passes itself around as user_data.

  static void
  foo_bar_async_op_cb (GObject *source_object,
                       GAsyncResult *res,
                       gpointer user_data)
  {
    MyObject *self;
    FooBar *bar = FOO_BAR (source_object);
    GError *error = NULL;

    if (!foo_bar_async_op_finish (bar, res, &error))
      {
        if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
          {
            /* self might be disposed */
            ...
            return;
          }
      }

     self = MY_OBJECT (user_data);
     ...
  }

  static void
  my_object_some_method (MyObject *self, ...)
  {
    ...
    foo_bar_async_op (bar, ..., self->cancellable, foo_bar_async_op_cb, self);
    ...
  }

  static void
  my_object_dispose (GObject *object)
  {
    MyObject *self = MY_OBJECT (object);

    if (self->cancellable != NULL)
      {
        g_cancellable_cancel (self->cancellable);
        g_clear_object (&self->cancellable);
      }
    ...
  }

You can find this pattern in lots of places in GNOME - gnome-control-center, gnome-settings-daemon, gtk+ and totem to name a few examples that weren't authored by me.

(One variant of this pattern is to pass the self->cancellable to the foo_bar_async_op_cb callback using the user_data, and use g_cancellable_is_cancelled. That will almost always require a custom struct or g_object_set_data. Otherwise you can't even pass "self". Not good.)

As you pointed out, one alternative is to use a GWeakRef, but that doesn't help with cancelling MyObject's outstanding internal operations.

Yes, one can also keep a reference on 'self', but that's non-ideal. It keeps self/MyObject alive even though there are no external users of it. If 'self' goes on to emit a signal then it can surprise any past users who didn't bother to use g_signal_connect_object because they thought they are the sole users of it. Bug 741257 is an example of this.

There are thousands of lines of code in GNOME where users of an object don't use g_signal_connect_object when they were the sole owners of it. The object is expected to get destroyed when the last reference is dropped. It is unreasonable to ask them to predict that the object might have funny semantics and live on. At the very least it would require knowledge about the object's implementation details.

It is also problematic to require MyObject to grow a "stop" method to kill the pending operations.

First, it leaks the implementation details of MyObject. What if MyObject stops using asynchronous operations internally in a future iteration of the code? Or, even worse, every object that doesn't have internal asynchronous operation should grow a stop method just in case they ever start doing so. Will GtkFileChooserDialog, GtkPlacesSidebar, etc. grow "stop" methods now?

It may work for simple cases where it is easy to know when the last reference is being dropped. Otherwise, the "stop" method will require custom reference counting inside MyObject to know when the last "stop" call was made. Not good.

--

We cannot claim that a libgdata API that accepts a GCancellable is not cancellable. If a user is not meant to interrupt it, then it shouldn't have accepted a GCancellable in the first place.

Let's not go about inventing semi-cancellable semantics. eg., it will throw G_IO_ERROR_CANCELLED if the GCancellable was triggered at the right time, otherwise not.

It is a different matter how the cancellation is handled. Resolving DNS, establishing a TCP connection, etc. are definitely cancellable because they don't change the state on the server. However, once a write/update message reaches the server, state change is inevitable. That sub-section of the operation should be considered atomic, and shouldn't be using a GCancellable. (Is libgdata atomic in this sense?) But these details are upto the libgdata implementation to deal with. Triggering a GCancellable doesn't mean that the overall operation has to be stopped instantaneously. It should be stopped whenever it is safe to stop it.

Regardless, to the external user, the overall operation is still cancellable. I understand the desire to let the application know about the server's state. But not at the cost of violating the GAsyncResult semantics.

Compare this with how the GFile API works. eg., if a g_file_replace is cancelled, it doesn't indicate whether the file was actually replaced, or the disk ran out space, or nothing happened.

Note that if a user is actually cancelling an operation, it might be that the user isn't interested in the actual/masked error anymore, as long as the data on the server isn't corrupted. On the other hand, if the actual status is really important, then chances are the user won't cancel the operation at all. eg., when a text editor is saving a text file, it needs to ensure that nothing short of a SIGTERM, or an explicit user action to cancel the write operation will get in the way of that operation running to completion. A random ctrl+q shouldn't silently result in data loss.

--

Yes, a GCancellable is thread-safe and can be triggered from any thread.

First of all, a significant percentage of code consuming GAsyncResult-based APIs is largely single threaded. One thread for interacting with the user, and worker threads and GSources to schedule tasks that take longer to finish. Hidding all these worker threads and GSources from the consumers of the GAsyncResult-based API is one of its biggest selling points. The concern about the GCancellable being triggered from a second thread is largely theoretical in such cases.

In cases where g_cancellable_cancel can be invoked from multiple threads, I'd expect them to share a critical section with the GAsyncReadyCallback. Otherwise, how would the g_cancellable_cancel callers know that self->cancellable has changed?

  static void
  foo_bar_async_op_cb (GObject *source_object,
                       GAsyncResult *res,
                       gpointer user_data)
  {
    MyObject *self;
    FooBar *bar = FOO_BAR (source_object);
    GError *error = NULL;

    g_mutex_lock (&mutex);

    if (!foo_bar_async_op_finish (bar, res, &error))
      {
        if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
          {
            /* self might be disposed */
            ...
            goto out;
          }
      }

     self = MY_OBJECT (user_data);
     self->running = FALSE;
     ...

   out:
     g_mutex_unlock (&mutex);
  }

  static void
  my_object_cancellation_from_first_thread (MyObject *self, ...)
  {
    g_mutex_lock (&mutex);
    ...
    if (self->running)
      {
        g_cancellable_cancel (self->cancellable);
        g_clear_object (&self->cancellable);
        self->cancellable = g_cancellable_new ();
      }
    ...
    g_mutex_unlock (&mutex);
  }

  static void
  my_object_cancellation_from_second_thread (MyObject *self, ...)
  {
    g_mutex_lock (&mutex);
    ...
    if (self->running)
      {
        g_cancellable_cancel (self->cancellable);
        g_clear_object (&self->cancellable);
        self->cancellable = g_cancellable_new ();
      }
    ...
    g_mutex_unlock (&mutex);
  }

  static void
  my_object_some_method (MyObject *self, ...)
  {
    g_mutex_lock (&mutex);
    ...
    self->running = TRUE;
    foo_bar_async_op (bar, ..., self->cancellable, foo_bar_async_op_cb, self);
    ...
    g_mutex_unlock (&mutex);
  }

  static void
  my_object_dispose (GObject *object)
  {
    MyObject *self = MY_OBJECT (object);

    if (self->cancellable != NULL)
      {
        g_cancellable_cancel (self->cancellable);
        g_clear_object (&self->cancellable);
      }
    ...
  }

--

To accommodate the desire to return the actual status of the operation on the server, we could use a custom implementation of GAsyncResult that's capable of tracking two GErrors. It could have a "get_server_status" method that returns a gboolean and throws the actual error from the server, if any.

--

As someone who maintains a bunch of code relying on libgdata in C and JavaScript, I'd really appreciate if the library adhered to the widespread rules and conventions around GAsyncResult in the GNOME platform. An application often ends up with long chains of asynchronous operations spanning multiple modules, and it is painful to deal with libraries that choose to come up with their own non-standard semantics.

I don't know if this particular issue is already causing problems somewhere or not. But if it does, then it will be
choice between writing my own wrapper or dropping the libgdata dependency. I hope it doesn't come to that.

--

[1] https://bugzilla.gnome.org/show_bug.cgi?id=684920#c262
[2] https://bugzilla.gnome.org/show_bug.cgi?id=684920#c272
[3] https://developer.gnome.org/gio/stable/GAsyncResult.html

From #gtk+ on GIMPNet:

14:33 <rishi> benzea: Company:                                                  
      https://bugzilla.gnome.org/show_bug.cgi?id=786282
14:33 <bugbot> Bug 786282: General, normal, libgdata-maint, NEW , Fix           
      cancellation support to use g_task_set_check_cancellable()
14:33 <rishi> pwithnall: I didn't want to pick on you. I am sorry if it         
      sounded like that. :)
14:34 <Company> rishi: I think that bug is either WONTFIX or NOTABUG

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libgdata/issues/22.