Bug 24768

Summary: add Observer.Recover flag to catch up on previous events (best-effort)
Product: Telepathy Reporter: Danielle Madeley <danielle>
Component: tp-specAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: morten.mjelva, smcv
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=shortlog;h=refs/heads/respawnable-observers
Whiteboard: review+ specmeet+
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 27309, 27619    

Description Danielle Madeley 2009-10-27 16:34:59 UTC
10:26 < danni> smcv: is there an official way for an observer to find out about 
               all existing channels?
10:27 < sjoerd> danni: nope
10:27 < sjoerd> (yes this is a weakness)
10:27 < danni> so is the solution "more API" or "ask AM for a list of 
               Connections and then iterate them for channels you care about"
10:28 < sjoerd> long-term or short-term ?
10:28 < danni> both?
10:28 < sjoerd> first is long-term, second is short-term
10:29 < danni> ok
10:29 < sjoerd> danni: if you could file a bug against tp-spec, that would be 
                great
Comment 1 Danielle Madeley 2009-12-28 17:23:32 UTC
Had a thought today. This could simply be a signal on the Observer interface that can Observer can emit, that way the CD can know the emitters path and have a look at the appropriate filter before calling ObserveChannels.
Comment 2 Will Thompson 2009-12-29 04:07:01 UTC
(In reply to comment #1)
> Had a thought today. This could simply be a signal on the Observer interface
> that can Observer can emit, that way the CD can know the emitters path and have
> a look at the appropriate filter before calling ObserveChannels.

I don't think it can be: how can the observer know that MissionControl is ready to receive its signals?
Comment 3 Simon McVittie 2010-03-16 05:58:41 UTC
12:55 < smcv> I think this is actually a special case of the "please restart 
              me" flag
12:56 < danni> smcv: you may not have been restarted of course, you may have 
               been started for the first time
12:56 < smcv> PleaseRestartMe && activatable -> (re)start the observer whenever 
              a channel it's interested in appears, or whenever it exits while 
              such a channel exists
12:56 < danni> and want to know what's happening
12:56 < smcv> PleaseRestartMe && !activatable -> whenever the observer pops up, 
              call ObserveChannels for each channel it would have been 
              interested in
12:57 < smcv> in any case, someone (ptlo?) is working on the former case, which 
              makes the latter case considerably simpler to implement even if 
              it ends up not being the same API

PleaseRestartMe is Bug #24376.
Comment 4 Simon McVittie 2010-03-16 06:04:42 UTC
If we implement this by calling ObserveChannels() again, there ought to be something in Observer_Info ({'catch-up': True} perhaps?) to indicate that they're not really new channels.
Comment 6 Simon McVittie 2010-03-25 07:06:29 UTC
+        <p>If true, upon the startup of this observer, ObserveChannels will be
+        called for every already existing channel that this observer wants to
+        observe (ie. every channel that would cause ObserveChannels to be called
+        on this observer upon creation).</p>

Insert a blank line between this paragraph and the next, please.

+        <p>If the observer is an activatable client, ObserveChannels won't be
+        emitted after the client is ready. Instead, it will be omitted immediately
+        when the observer's well-known name disappears from the bus, so the client
+        will be restarted immediately via DBus service activation.</p>

Methods are "called", not "emitted". They're certainly not "omitted", which is something else entirely :-)

Also, I think this wording is a bit confusing. How about this?

         <p>If the Observer is service-activatable, instead of watching
           passively for a new instance of it to start, the channel
           dispatcher will call its ObserveChannels method as soon as
           its well-known name disappears, resulting in it being restarted
           immediately via D-Bus service activation.</p>

... and I'd like some rationale, like this:

         <tp:rationale>
           <p>This means that if an activatable Observer crashes, it will
             be restarted as soon as possible; while there is an unavoidable
             possibility that it will miss some events during this process
             (particularly <tp:dbus-ref 
               namespace="org.freedesktop.Channel.Type">Text</tp:dbus-ref>
             messages), this window of event loss is kept to a minimum.</p>

           <p>Non-activatable observers can't take advantage of this
             mechanism, but setting this property on a non-activatable
             observer does allow it to "catch up" on channels that are
             currently active at the time that it starts up.</p>
         </tp:rationale>
Comment 7 Simon McVittie 2010-03-25 07:17:54 UTC
>        <p>When the ObserveChannels method is called due to observer recovery,
>        the "Observer_Info" dictionary will contain one extra item with key
>        "ExistingChannel" and boolean value of True.</p>

Our case convention for "extra misc info" dicts is usually like GObject properties (so, "existing-channel" in this case).

I think this key should be called "recovering", actually.

The Observer_Info parameter to ObserveChannels should document it, too. Pseudocode, in which /foo/ indicates a hyperlink:

    recovering : b
        If present and TRUE, this channel already existed before the observer
        started, and is being supplied to it due to the /Recover/ property
        (see that property for details).

>        <tp:rationale>
>          <p>The observers might hold state or do channel-specific initialisation
>          for the channels they observe. This is a way of signalling to them
>          that they were already observing this channel, so they don't have to
>          do extra round-trips to query for this information.</p>

I think this rationale is wrong. If you receive a channel with 'recovering', then by definition you've just started up, so you have no state!

Instead, the rationale for the "recovering" key should be something like:

    This allows observers to distinguish between new channels (the normal
    case), and existing channels that were given to the observer in order
    to catch up on previous events (perhaps after a previous instance of
    the same observer crashed).

Do you have an implementation of this in MC? I'll clone this bug for the implementation; please attach any implementation for review to the clone, with the patch keyword.

This spec change can potentially be merged without an implementation in MC, but can't be undrafted until we have a suitable implementation. To be honest, since this branch is so small, I'd be inclined to implement it in MC based on this draft, and not bother to merge it to the spec until we have a reasonable implementation on a branch, at which point it can go straight in as stable API.

When this is ready for review again, please remove review- from the status whiteboard and re-add the patch keyword.
Comment 8 Simon McVittie 2010-03-25 07:21:36 UTC
*** Bug 24376 has been marked as a duplicate of this bug. ***
Comment 9 Simon McVittie 2010-03-29 04:45:57 UTC
12:33 < ptlo> 12:22:50> smcv, wrt fd.o comment #24768, I didn't document the 
              recover flag in Observer_Info doc because that's in 
              Client.Observer (which AIUI is not to be touched with unstable 
              features), not Client.Observer.FUTURE; it didn't seem right to 
              copy/paste the entire ObserveChannels/Observer_Info doc into 
              .FUTURE just to be able to add the flag there

That's fair enough, but please provide (either on this bug, or somewhere in the branch - e.g. text headed <strong>Text to be added to Observer_Info</strong> in the FUTURE XML, or something) the text that will be *added* to Observer_Info, so that undrafting this feature is just a matter of moving text around.
Comment 10 Senko Rasic 2010-04-13 04:48:19 UTC
(In reply to comment #9)
> 12:33 < ptlo> 12:22:50> smcv, wrt fd.o comment #24768, I didn't document the 
>               recover flag in Observer_Info doc because that's in 
>               Client.Observer (which AIUI is not to be touched with unstable 
>               features), not Client.Observer.FUTURE; it didn't seem right to 
>               copy/paste the entire ObserveChannels/Observer_Info doc into 
>               .FUTURE just to be able to add the flag there
> 
> That's fair enough, but please provide (either on this bug, or somewhere in the
> branch - e.g. text headed <strong>Text to be added to Observer_Info</strong> in
> the FUTURE XML, or something) the text that will be *added* to Observer_Info,
> so that undrafting this feature is just a matter of moving text around.

Changed according to review comments, squashed into a single commit:

http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=commitdiff;h=9c0ab8aa46108d63980192e9cd955e8a9f99d2ff
Comment 11 Simon McVittie 2010-04-13 10:18:33 UTC
(In reply to comment #10)
> http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=commitdiff;h=9c0ab8aa46108d63980192e9cd955e8a9f99d2ff

"MailNotification: UnreadMailCount >= UnreadMails array size"? I don't think that's the patch you meant :-)

Assuming that <http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=shortlog;h=refs/heads/respawnable-observers> is what you meant me to review, I approve.

However, since this is such a small spec change, it may be easier to not bother merging it as a draft - we could probably just insta-undraft it, if there's a good implementation already, so I'll review the MC implementation of this feature next.
Comment 12 Simon McVittie 2010-04-13 10:40:01 UTC
The second try at an implementation over on Bug #27309 is good enough to reassure me that this API is implementable, so I think we can go directly to an undrafted version of this.
Comment 13 Senko Rasic 2010-04-14 04:33:31 UTC
(In reply to comment #12)
> The second try at an implementation over on Bug #27309 is good enough to
> reassure me that this API is implementable, so I think we can go directly to an
> undrafted version of this.

Here's the undrafted version for instareview:

http://git.collabora.co.uk/?p=user/ptlo/tp-spec-senko/.git;a=commitdiff;h=871718924be6e3bcaf4ffb7f0e7aafe75462b245
Comment 14 Simon McVittie 2010-04-14 04:37:39 UTC
The undrafted version looks good to me, but please don't merge without spec-meeting approval. Thanks for getting this working!
Comment 15 Simon McVittie 2010-04-15 08:22:42 UTC
Spec cabal approves, I'll merge it for the next spec release.
Comment 16 Simon McVittie 2010-04-15 13:54:57 UTC
Stable API in 0.19.4.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.