Bug 47322

I actually like the weak object approach. What's the right pattern when using GCancellable? Having a self->priv->cancellable passed to all _async() calls and cancelled in the dipose method?

Yes, I believe that's how GCancellable is meant to be used.

(In reply to comment #0)
> In the old generated APIs, we had weak_objects [...]
> The solution is to use GCancellable

Is this the GIO policy? It'd be worth asking the designers of GIO before enforcing this globally. I'm not sure that this is appropriate for things that don't have a user-visible side-effect or can't actually be cancelled.

If you want to avoid dereferencing a dangling pointer to a freed object, GCancellable isn't suitable anyway, as far as I can see, because functions taking a cancellable are free to ignore it - it's best-effort cancellation, not guaranteed cancellation.

However, you can get the same effect as weak_object by using a TpWeakObject as your user-data, and a sequence like this:

  TpThing *self = tp_weak_ref_dup_object (user_data);

  if (self == NULL)
    return;

  g_assert (TP_IS_THING (self));
  /* continue with the async callback */

My understanding was that once you call cancel, your other callback is guaranteed to never be called, but maybe I got it wrong as the expected general working of GIO isn't documented.

But yea, the new GWeakRef is probably what I want..

(In reply to comment #4)
> My understanding was that once you call cancel, your other callback is
> guaranteed to never be called

No, in GIO (unlike older bits of telepathy-glib) your callback is *always* called with either success or error. Cancellables don't change that.

If the cancellable was cancelled, the operation might fail with G_IO_ERROR_CANCELLED, or it might succeed or fail with some other error anyway.

> But yea, the new GWeakRef is probably what I want..

GWeakRef isn't what you want on its own - it isn't (necessarily) heap-allocated, so you'd have to wrap it with e.g. g_new0 (GWeakRef) and g_weak_ref_clear/g_free.

TpWeakRef is basically just a heap-allocated weak ref with a secondary user_data pointer, designed to make it easy to implement our old weak_object semantics in terms of GAsyncResult calls. Patches welcome to make it use GWeakRef instead of g_object_add_weak_pointer, if you want to go that route.

I it might not be all method, but at least all the DBus call, especially that it's free.

While cancelling does not prevent you from being callback, it help you make the difference between a normal error and a abnormal error. We can greatly improve traces with that, and avoid all the obscure NULL check in every callback.

Also, in some cases, async call will end much later without cancellable. This can make debugging of end-of-life races much harder. Cancellable is simply to help you make sure your now unwanted call ends in a finit amount of time. An example where I want Cancellable is in telepathy Logger, some searches can take more then 30 seconds to finish, for sure it's going to be cancelled on best effort, but most likely within less then 1 seconds. This is a big deal of disk resource that you save for the next search.

(In reply to comment #6)
> I it might not be all method, but at least all the DBus call, especially that
> it's free.

For APIs that make more than one method call behind the scenes (like a Telepathy ChannelRequest), you can cancel and it will actually work, but to do that, you need to design your D-Bus API to allow for it, something like:

    method Start()
    signal MoreResults(...)
    method Stop()

Again, ChannelRequest, FileTransfer and ContactSearch can do this, and perhaps so can telepathy-logger (I haven't reviewed its API). For these APIs, sure, the async op should be cancellable.

Most of Telepathy can't do this, though, because its D-Bus calls are just send method call / wait for reply. If the method-call message has already been sent to the service, there's nothing you can do about it; its side-effects will happen whether you still want them or not.

If the D-Bus method-call message is still in your outgoing queue, in principle you could remove it from that queue. I'm not aware of any implementation that actually lets you do that, though (neither libdbus nor GDBus has this).

GDBus claims that method call are cancellable, but if you cancel, all it does is to make a best-effort attempt to pass G_IO_ERROR_CANCELLED to the callback "soon". The method-call message is still sent, and still has its side-effects. I think this is quite misleading, and potentially dangerous.

The current tp_proxy_pending_call_cancel() (which is effectively what the death of a weak_object does) doesn't call your callback at all, but other than that, behaves like GDBus. Again, I think this is misleading.

(In reply to comment #6)
> Also, in some cases, async call will end much later without cancellable. This
> can make debugging of end-of-life races much harder.

This is a valid reason to want "fake cancellability" for D-Bus calls. I'm not sure which is more important: this (in favour of cancellability), or the fact that the side-effects of the call will still happen (against cancellability).

(In reply to comment #5)
> (In reply to comment #4)
> > My understanding was that once you call cancel, your other callback is
> > guaranteed to never be called
> 
> No, in GIO (unlike older bits of telepathy-glib) your callback is *always*
> called with either success or error. Cancellables don't change that.
> 
> If the cancellable was cancelled, the operation might fail with
> G_IO_ERROR_CANCELLED, or it might succeed or fail with some other error anyway.

In this case, are we suppose to compleet the operation in idle or not. Because if we don't I don't see how that help for the "please don't call my callback on a disposed object" case.

(In reply to comment #9)
> In this case, are we suppose to compleet the operation in idle or not.

Same rules as completing any other async op: in an idle, unless you can guarantee that you're being called directly from the appropriate main context already.

> Because
> if we don't I don't see how that help for the "please don't call my callback on
> a disposed object" case.

It doesn't. You have to use weak pointers and check for invalidation yourself, regardless.

I was suspecting that. So yeah, I really don't see the point of having GCancellable in most of our API.

Well, the designed way was that a GAsyncResult would have a reference on the callback context. For sure this will delay dispose until all the async method have been callback, and the CANCELLED error is to tell you so.

You can use WeakRef is you absolutely need to dispose now (e.g. you're a GStreamer element going to NULL), but imho in Telepathy this is never required. We do it this way in Psyke btw.

I think this is pretty clear now that cancellables don't fit in very well here. Having a cancellable around makes its purpose extremely misleading given the side-effects still happen.

(In reply to comment #7)
> For APIs that make more than one method call behind the scenes (like a
> Telepathy ChannelRequest), you can cancel and it will actually work, but to do
> that, you need to design your D-Bus API to allow for it, something like:
> 
>     method Start()
>     signal MoreResults(...)
>     method Stop()
> 
> Again, ChannelRequest, FileTransfer and ContactSearch can do this, and perhaps
> so can telepathy-logger (I haven't reviewed its API). For these APIs, sure, the
> async op should be cancellable.

Hmm, really? Say we have an interface called Bong with those three members, is this the kind of thing you want?

    // calls Start
    void tp_bong_get_results_async(TpBong *,
                                   GCancellable *,
                                   GAsyncResultCallback,
                                   gpointer);

    // fired when MoreResults is emitted
    signal TpBong::got-results(TpBong *,
                               TpBongResult *,
                               gpointer)

    // calls Stop
    GList * tp_bong_get_results_finish(TpBong *,
                                       GAsyncResult *,
                                       GError **);

(or _finish could just not return anything)

So you get an update for any new results so you can update the UI in real time. Cancelling the GCancellable when you've had enough (or the result you wanted appeared so the user picked it and closed the dialog, say) is reasonable.

What doesn't feel so right to me is that the async operation taking potentially minutes. Is this an unnecessary concern of mine?

Anyway, this is a pointless discussion given most of the API isn't designed like this — I notice TpContactSearch doesn't use this design or GCancellables.

I'm going to mark this as WONTFIX.