Bug 21097

I'm working on it, and have a branch that adds (core and optional) feature support to TpProxy, and ports TpChannel to use it.

Remaining things to do for this to be reviewable:

* Define and design the interaction with TpConnection statuses

* Port TpAccount, TpAccountManager to use the generic TpProxy feature stuff, with their own methods maintained for compatibility

* Port examples, etc. to use prepare_async rather than readying

Actively working on this; I have branches that convert TpChannel, TpConnection, TpAccount, TpAccountManager and TpConnectionManager (i.e. all the classes that implement readying or features of any sort), but have realised that they're doing GCancellable wrong, so need re-doing.

I think this should now be ready for review. It's stacked on top of smcv/weakref, which TpConnectionManager now uses internally (so please review that first).

TpConnectionManager doesn't make use of the spare user_data gpointer in TpWeakRef, because it takes an *optional* weak object, so can't rely on having a TpWeakRef to put its user_data in. I expect the spare gpointer to be useful for library-user code that *always* has a weak object.

Weakref is only http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=aad1b67b938328f830984329fc950e238c1f96f3 right ?

Review of this commit:

+    gpointer magic;
A comment explaining the semantic of this pointer would be good.


+TpWeakRef *tp_weak_ref_new (gpointer object, gpointer user_data,
That's now how you do coding style :p

What's the rational of having a dup_object() and no get_object()? In most case the object will be the 'self' pointer of the object who started the async call so getting an extra ref on itself is not interesting and just add an 'extra' g_object_unref.

(In reply to comment #4)
> Weakref is only
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=aad1b67b938328f830984329fc950e238c1f96f3
> right ?

Correct.

> +    gpointer magic;
> A comment explaining the semantic of this pointer would be good.

Yeah, I'll add one. It's just an arbitrary "magic number" to detect use-after-free or whatever (and it makes the size of the struct a power of 2, which is probably not harmful? I'm not sure how g_slice_new pads things).

> +TpWeakRef *tp_weak_ref_new (gpointer object, gpointer user_data,
> That's now how you do coding style :p

It is in declarations, unless you're enforcing a stricter policy for header files than we currently do (which I wouldn't necessarily oppose). /Style on the wiki is ambiguous, and vaguely in favour of your stricter policy.

> What's the rational of having a dup_object() and no get_object()? In most case
> the object will be the 'self' pointer of the object who started the async call
> so getting an extra ref on itself is not interesting and just add an 'extra'
> g_object_unref.

The rationale is that this is the safe way to do weak refs. As long as only a weak ref is held, the object's last ref could be released at any time, by any call into user-supplied code (notably, any signal emission!); temporarily strengthening the ref fixes this.

I could add a get_object version with a big fat warning, but I fear that the result of that would be that everyone used the get_object version regardless of what the documentation said, and had hidden use-after-frees...

(The other way to do this would be some sort of GClosure-based magic around the callback, but that makes it awkward to use with existing callbacks.)

(In reply to comment #5)
> > +    gpointer magic;
> > A comment explaining the semantic of this pointer would be good.
> 
> Yeah, I'll add one. It's just an arbitrary "magic number" to detect
> use-after-free or whatever (and it makes the size of the struct a power of 2,
> which is probably not harmful? I'm not sure how g_slice_new pads things).

I'm not really convinced it's worth it as it doesn't give us anything.

> > +TpWeakRef *tp_weak_ref_new (gpointer object, gpointer user_data,
> > That's now how you do coding style :p
> 
> It is in declarations, unless you're enforcing a stricter policy for header
> files than we currently do (which I wouldn't necessarily oppose). /Style on the
> wiki is ambiguous, and vaguely in favour of your stricter policy.

I always apply a "one arg per line" policy in declarations assuming it was in the policy but I don't care that much tbh.

+gboolean _tp_proxy_is_preparing (gpointer self, GQuark feature);
+void _tp_proxy_set_feature_prepared (TpProxy *self, GQuark feature,
+    gboolean succeeded);

+static void tp_proxy_poll_features (TpProxy *self, const GError *error);

Should be one line per arg.

+    FEATURE_STATE_INVALID = GPOINTER_TO_INT (NULL),
Any reason to not use "= 0" ?

Is it sane to allow to call tp_proxy_prepare_async with a NULL callback?

Shouldn't we have TP_CHANNEL_FEATURE_GROUP rather than including it with CORE?

tp_channel_is_ready's doc:
+ * This is a less general form of tp_proxy_is_prepared(), which should be
+ * used in new code.
should NOT be used. Or maybe I miss-parsed this comment?
Same comment for TpChannel:channel-ready

I guess it's ok to call tp_cli_connection_call_connect on a not prepared TpConnection right?

Shouldn't we deprecate old API?

Probably a bit out of scope for this branch, but as an API user I have to say that I'd really love to have a way to ask "prepare this object and the objects it's owning as well".
For example "prepare the account manager and the accounts as well". Maybe we could have object specific API for that? Or having a TP_ACCOUNT_MANAGER_FEATURE_CHANNEL_CORE ? In that case we won't be able to ask for channels specific features.

(In reply to comment #7)
> +    FEATURE_STATE_INVALID = GPOINTER_TO_INT (NULL),
> Any reason to not use "= 0" ?

The code relies on GPOINTER_TO_INT (g_hash_table_lookup (ht, an_absent_key)) returning FEATURE_STATE_INVALID. In practice we (and most other C libraries) will fail horribly on any platform where NULL isn't all-bits-zero, though.

> Is it sane to allow to call tp_proxy_prepare_async with a NULL callback?

TpAccount allows it, and it doesn't seem harmful. The use-case is fairly tenuous (getting a head-start on preparing a feature that someone else will later wait for).

> Shouldn't we have TP_CHANNEL_FEATURE_GROUP rather than including it with CORE?

In the current TpChannel code, new channels are "trying to prepare" the group stuff, even if nobody asked for it. I'm inclined to think that that's an API guarantee.

I suppose it might make sense to have GROUP as a separate feature so CORE doesn't have to wait for it... but the current code will wait for both anyway, because there's a single introspect pipeline.

> tp_channel_is_ready's doc:
> + * This is a less general form of tp_proxy_is_prepared(), which should be
> + * used in new code.
> should NOT be used. Or maybe I miss-parsed this comment?
> Same comment for TpChannel:channel-ready

You did mis-parse, but I agree it could be clearer. The intended meaning was "This is a less general form of t_p_i_p(); t_p_i_p() should be used in new code", but I think that's an unwieldy way to phrase it.

Perhaps "New code should use t_p_i_p(), which is a more general form of this method"?

> I guess it's ok to call tp_cli_connection_call_connect on a not prepared
> TpConnection right?

Yes, the TpConnection already knows that it's a Connection.

> Shouldn't we deprecate old API?

Not yet, I don't think; deprecation is rather a big stick (it breaks all of our development builds, because they have -Werror and -Wdeprecated-declarations), and we probably shouldn't deprecate API for which the replacement requires a bleeding-edge dependency. I'd be inclined to deprecate the call_when_ready family in 0.13.0.

It's an unfortunate situation... I'd like to have a way to get non-fatal warnings for things that aren't yet fully deprecated, but are on the way there (like tp_get_bus(), premature deprecation of which broke the buildbot today).

> Probably a bit out of scope for this branch, but as an API user I have to say
> that I'd really love to have a way to ask "prepare this object and the objects
> it's owning as well".

Yes, we should have that; yes, it's out of scope; and no, I can't think of a nice way to express "prepare all of this AM's accounts, with the AVATAR feature".

I think this should be done case-by-case rather than as a general thing: it makes sense to want to prepare all of an AM's Accounts, but I don't think it makes sense to want to prepare all of a Connection's Channels.

Plural operations that might be worth having a shorthand feature later:

* given the AccountManager, prepare all valid Accounts
* given the AccountManager, prepare all valid and invalid Accounts
* given a ChannelDispatchOperation, prepare the Account, the Connection and all of the Channels
* given a ChannelDispatcher with the OperationList interface, prepare all the CDOs
* given an Account, prepare the ConnectionManager and the (future) Protocol
* given a ChannelRequest, prepare the Account

(In reply to comment #7)
> +gboolean _tp_proxy_is_preparing (gpointer self, GQuark feature);
> +void _tp_proxy_set_feature_prepared (TpProxy *self, GQuark feature,
> +    gboolean succeeded);
> 
> +static void tp_proxy_poll_features (TpProxy *self, const GError *error);
> 
> Should be one line per arg.

I'll do this for the whole branch at once, if at all. I've updated weakref with removal of @magic - are you satisfied by my rationale for having dup_object and no get_object?

(In reply to comment #5)
> The rationale is that this is the safe way to do weak refs. As long as only a
> weak ref is held, the object's last ref could be released at any time, by any
> call into user-supplied code (notably, any signal emission!); temporarily
> strengthening the ref fixes this.

I'm not sure to understand this. If the callback is called the object is still alive right? So, how can it be destroyed while I'm in the callback? Or are you considering multi-threated code?

> You did mis-parse, but I agree it could be clearer. The intended meaning was
> "This is a less general form of t_p_i_p(); t_p_i_p() should be used in new
> code", but I think that's an unwieldy way to phrase it.
> 
> Perhaps "New code should use t_p_i_p(), which is a more general form of this
> method"?

Yeah I prefer this phrasing.

> > I guess it's ok to call tp_cli_connection_call_connect on a not prepared
> > TpConnection right?
> 
> Yes, the TpConnection already knows that it's a Connection.
> 
> > Shouldn't we deprecate old API?
> 
> Not yet, I don't think; deprecation is rather a big stick (it breaks all of our
> development builds, because they have -Werror and -Wdeprecated-declarations),
> and we probably shouldn't deprecate API for which the replacement requires a
> bleeding-edge dependency. I'd be inclined to deprecate the call_when_ready
> family in 0.13.0.
> 
> It's an unfortunate situation... I'd like to have a way to get non-fatal
> warnings for things that aren't yet fully deprecated, but are on the way there
> (like tp_get_bus(), premature deprecation of which broke the buildbot today).

I'm not sure. Depreacting API is not that bad, you can still easily remove the deprecated flag if needed and it's useful to spot code using the old API. AFAIK, GTK+ deprecated API as soon there is a replacement for them and that works fine.

(In reply to comment #10)
> (In reply to comment #5)
> > The rationale is that this is the safe way to do weak refs. As long as only a
> > weak ref is held, the object's last ref could be released at any time, by any
> > call into user-supplied code (notably, any signal emission!); temporarily
> > strengthening the ref fixes this.
> 
> I'm not sure to understand this. If the callback is called the object is still
> alive right? So, how can it be destroyed while I'm in the callback? Or are you
> considering multi-threated code?

* Badger calls an async method, with a TpWeakRef for itself as user_data
* Mushroom holds the last ref to Badger, and is connected to Badger::oscillating
* The callback for the async method looks like this:

  badger_connection_prepared_cb (...)
  {
    Badger *self = tp_weak_ref_get_object (user_data);

    ...
    g_signal_emit_by_name (self, "oscillating", 60 /* Hz */);
    ...
    badger_check_for_snakes (self);
  }

* In the callback for Badger::oscillating, Mushroom releases the reference it had
* The call to badger_check_for_snakes() is now a use-after-free

Good point. I'm convinced now. :)

I've merged weakref based on Guillaume's approval. Further actions required from me:

(In reply to comment #8)
> (In reply to comment #7)
> > Shouldn't we have TP_CHANNEL_FEATURE_GROUP rather than including it with CORE?

I'll consider this, but GROUP always needs to be "trying to prepare".

> > tp_channel_is_ready's doc: 
> Perhaps "New code should use t_p_i_p(), which is a more general form of this
> method"?

I'll make this change.

(In reply to comment #9)
> (In reply to comment #7)
> > +gboolean _tp_proxy_is_preparing (gpointer self, GQuark feature);
> > +void _tp_proxy_set_feature_prepared (TpProxy *self, GQuark feature,
> > +    gboolean succeeded);
> > 
> > +static void tp_proxy_poll_features (TpProxy *self, const GError *error);
> > 
> > Should be one line per arg.

Yeah, yeah, OK.

Guillaume/other reviewers, are you convinced by my reasoning for the other stuff that I don't plan to change?

I still think it would be good to have a way to turn deprecation of old API to easily spot place we are using them and avoid to introduce them in new code.

(In reply to comment #8)

> > Shouldn't we have TP_CHANNEL_FEATURE_GROUP rather than including it with CORE?
> 
> In the current TpChannel code, new channels are "trying to prepare" the group
> stuff, even if nobody asked for it. I'm inclined to think that that's an API
> guarantee.
> 
> I suppose it might make sense to have GROUP as a separate feature so CORE
> doesn't have to wait for it... but the current code will wait for both anyway,
> because there's a single introspect pipeline.

Since this is new API is there really any API guarantees? tp_channel_call_when_ready() could still prepare both features.

Even if this code currently prepared both features on the sly, anyone who had only prepared one feature, and was depending without explicitly preparing it has just written a buggy program.

tp_connection_manager_call_when_ready() has been ported to tp_proxy_prepare_async(), but tp_connection_call_when_ready() and tp_channel_call_when_ready() have not.

(In reply to comment #17)
> tp_connection_manager_call_when_ready() has been ported to
> tp_proxy_prepare_async(), but tp_connection_call_when_ready() and
> tp_channel_call_when_ready() have not.

That's because their semantics are not the same. If the proxy is already ready, tp_proxy_prepare_async() calls you back from an idle, but tp_channel_call_when_ready() and tp_connection_call_when_ready() call you back immediately.

(In reply to comment #18)
> (In reply to comment #17)
> > tp_connection_manager_call_when_ready() has been ported to
> > tp_proxy_prepare_async(), but tp_connection_call_when_ready() and
> > tp_channel_call_when_ready() have not.
> 
> That's because their semantics are not the same. If the proxy is already ready,
> tp_proxy_prepare_async() calls you back from an idle, but
> tp_channel_call_when_ready() and tp_connection_call_when_ready() call you back
> immediately.

That couldn't still be implemented using is_prepared()?

I've gone through this branch twice now and it works with my gobject-introspection work. It looks fine to me.

(In reply to comment #20)
> I've gone through this branch twice now and it works with my
> gobject-introspection work. It looks fine to me.

Thanks, merged. It'll be in 0.11.3.