Bug 27309

MC branch implementing this:

http://git.collabora.co.uk/?p=user/ptlo/telepathy-mission-control/.git;a=shortlog;h=refs/heads/observer-recovery

As of commit da0125, branch is missing a test for respawning activatable channels (I'm struggling with testsuite handling multiple dbus connections). Also note as of that commit, it's testing for 'ExistingChannel' instead of 'recovering' key in Observer_Info.

> +    master = mcd_master_get_default ();
> +    dispatcher = mcd_master_get_dispatcher (master);

Sorry, but I think this reduces MC's maintainability. I'm trying to phase out the layering violations in MC: please don't introduce more.

(It's possible that mcd_master_get_default() ought to be marked as deprecated within MC itself, to discourage this sort of thing.)

The layering currently goes like this, if I remember correctly:

McdDispatcher > McdDispatchOperation > McdHandlerMap
McdDispatcher > McdDispatchOperation > McdClientRegistry > McdClientProxy

(where ">" means "is bigger than/may reference/may call methods on", and is transitive)

Similarly, the ChannelDispatcher and the AccountManager should have a minimum of shared state, to reduce crazy interactions between the two (eventual goal: MC can be an AM and/or a CD, independently talking to the other half via D-Bus if it's non-local).

A couple of possible designs:

* Clients gain a reference to the handler map. The handler map gains a map from channel object path to account object path (whose keys you could use as the list of channels to recover, probably).

* Clients emit a signal when they want recovery. The dispatcher catches that signal, and calls some method on the clients for each matching channel (in practice, this would probably require the same enhancements to the handler map).

You should hopefully also find that at least some of _mcd_client_recover() (and the various helpers it calls) is duplicated by some existing code that could be factored out (one example below).

------------------------

Minor things:

I'd prefer it if you hadn't checked in a commit that assumes a TpChannel is a McdChannel (i.e. clearly doesn't work) followed by one that fixes it - rebase and squash the two?

> +        g_object_get (conn, "object-path", &connection_path, NULL);

Please put one key/value per line when calling g_object_get or g_object_new, even if there are 0 or 1 pairs.

In this case you could have just used tp_proxy_get_object_path(), though - using convenience "C bindings" like tp_proxy_get_object_path() and tp_channel_borrow_connection() is encouraged.

> +_build_chan_details (TpChannel *chan)

This could benefit from tp_value_array_build(). You could also move it to some sort of "channel utilities" module, and use it in the implementation of the McdChannel pseudo-method _mcd_channel_details_build_from_list().

(I'd suggest channel-utils.[ch] and the namespace _mcd_tp_channel_foo() for such a file; _mcd_tp_channel_should_close() could move there from mcd-channel-priv.h.)

> +GList* _mcd_handler_map_get_handled_channels (McdHandlerMap *self);

Nitpicking: swap the first * and the first space

(In reply to comment #1)
> As of commit da0125, branch is missing a test for respawning activatable
> channels (I'm struggling with testsuite handling multiple dbus connections).

The only way to be activatable is to install a .service file where MC will look (I think that means test/twisted/tools/ or something?) - to do this without interfering with other tests, you should probably use a unique dummy channel type ("ObserverRestartTest" or something) for all channels in this test.

> Also note as of that commit, it's testing for 'ExistingChannel' instead of
> 'recovering' key in Observer_Info.

I don't see where you set this in the C code? :-)

Senko, I see you have a 10-day-old branch for this with one big temporary commit. Any progress on breaking this up, or do you want it reviewed as a single large commit? Please ping this bug when it's ready for review.

(In reply to comment #2)
> The layering currently goes like this, if I remember correctly:
> 
> McdDispatcher > McdDispatchOperation > McdHandlerMap
> McdDispatcher > McdDispatchOperation > McdClientRegistry > McdClientProxy
> 
> (where ">" means "is bigger than/may reference/may call methods on", and is
> transitive)

Refactored according to this design.

> * Clients emit a signal when they want recovery. The dispatcher catches that
> signal, and calls some method on the clients for each matching channel (in
> practice, this would probably require the same enhancements to the handler
> map).

This sounds like the best idea to me, so I implemented that.

> You should hopefully also find that at least some of _mcd_client_recover() (and
> the various helpers it calls) is duplicated by some existing code that could be
> factored out (one example below).

Refactored a few things; one that I didn't is the actual call to observe_channels - while trying to make a common implementation that both DO and dispatcher while respawning could use, I found that it took the same amount of code to prepare the arguments (since it's done differently in the two cases) to not warrant the single implementation.

> I'd prefer it if you hadn't checked in a commit that assumes a TpChannel is a
> McdChannel (i.e. clearly doesn't work) followed by one that fixes it - rebase
> and squash the two?

In the newly updated branch, I've rebased the entire changeset and split it up into logical changes.

> 
> > +        g_object_get (conn, "object-path", &connection_path, NULL);
> 
> Please put one key/value per line when calling g_object_get or g_object_new,
> even if there are 0 or 1 pairs.

Done so.

> > +_build_chan_details (TpChannel *chan)
> 
> This could benefit from tp_value_array_build(). You could also move it to some

Tried, and found it didn't look any more clearer or less code (in fact, looks a bit more implicit IMHO). 

> sort of "channel utilities" module, and use it in the implementation of the
> McdChannel pseudo-method _mcd_channel_details_build_from_list().
> 
> (I'd suggest channel-utils.[ch] and the namespace _mcd_tp_channel_foo() for
> such a file; _mcd_tp_channel_should_close() could move there from
> mcd-channel-priv.h.)

Done so.

> > +GList* _mcd_handler_map_get_handled_channels (McdHandlerMap *self);
> 
> Nitpicking: swap the first * and the first space

Good catch of a typo, thanks.

(In reply to comment #3)
> The only way to be activatable is to install a .service file where MC will look
> (I think that means test/twisted/tools/ or something?) - to do this without
> interfering with other tests, you should probably use a unique dummy channel
> type ("ObserverRestartTest" or something) for all channels in this test.

Right, added a dummy "Logger" client observing "org.freedesktop.Telepathy.Channel.Type.RespawnObservers".

(In reply to comment #4)
> commit. Any progress on breaking this up, or do you want it reviewed as a
> single large commit? Please ping this bug when it's ready for review.

I've split it in multiple logical changes, and is ready for review (as of commit 4f4b1f3):

http://git.collabora.co.uk/?p=user/ptlo/telepathy-mission-control/.git;a=shortlog;h=refs/heads/observer-recovery

(patch is a valid keyword on this bugzilla - if you put it in Keywords rather than Whiteboard it'll show up in searches. Please do!)

Thanks, this structure is much better. Some minor changes:

> +_mcd_handler_map_get_channel_accounts (McdHandlerMap *self)

If possible I'd prefer a more "lookup-like" API for this. It looks as though all you use it for is to look up a channel path and return the account path, so perhaps turn it into const gchar *_mcd_handler_map_get_account (McdHandlerMap *, const gchar *)?

> @@ -604,8 +605,10 @@ _mcd_client_recover_observer (McdClientProxy *self, TpChannel *channel,
...
> +    g_object_get (channel,
> +                  "connection", &conn,
> +                  NULL);

This leaks a ref to conn. If you replace it with tp_channel_borrow_connection() it'll be simpler, more efficient, and no longer leaky.

This branch also complies with non-FUTURE spec that hasn't been merged yet, so shouldn't really be merged until this stuff is in the spec. However, it's good enough to reassure me that putting this stuff in the spec would be OK.

(In reply to comment #6)
> (patch is a valid keyword on this bugzilla - if you put it in Keywords rather
> than Whiteboard it'll show up in searches. Please do!)
> 
> Thanks, this structure is much better. Some minor changes:

Here's the updated branch (last two commits) with the changes:

http://git.collabora.co.uk/?p=user/ptlo/telepathy-mission-control/.git;a=shortlog;h=refs/heads/observer-recovery

This looks good to merge as soon as telepathy-spec supports it.

I ended up rebasing this, because we want it in 5.4.0. Will be fixed in 5.4.0.