[Telepathy] TelepathyQt4: State of the Connection

[Andre Moreira Magalhaes]

Hi,

Olli Salli wrote:
>> More on how I would implement this in the next mail.
>>     
>
> Currently, everything doing state download inside Connection revolves
> around a central "introspection queue". Functions to introspect items
> on the Connection are put into this queue, and fired one by one, when
> <something> decides this should be done. When all are processed, the
> Connection is ready. This was fine for the pre-features case, where:
>   - There was a clear chain of what should be introspected, and in which order
>   - Only getting a Connection status (either the first one, or a
> changed one) fired off introspection
>   - Introspection was advanced by to the next item by getting one item
> in the chain of items to introspect introspected, and only that
>
> However, this doesn't bode too well in the featureful case, where:
>   - additionally, becomeReady needs to fire off introspection, at an
> arbitrary time!
>     - in the current implementation, if introspection is already
> running, can make it advance to the next item before the previous one
> has been completed!
>       - could be hackily solved by adding an isIntrospecting boolean,
> but maintaining that would be rather error-prone
>   - stuff to introspect depends on the features requested
>     - it's VERY MESSY and TERRIBLY ERROR-PRONE to hard-code
> introspecting the "sensible next one" to every possible place where
> one item might have been introspected
>       - that "sensible next one" depends on which features exactly
> have been requested, and what other things they might depend on
>         - example: SelfContact depends on a) being online b) your self
> handle c) CAI being introspected
>   - features requested can increase at any point
>     - makes obsolete any checks done by previous introspection
> callbacks to queue new introspectable items based on what the
> requested features were at that point
>   - features only make sense with certain Connection interfaces present
>
> Needless to say, this leads to heavy complexity in all Connection
> logic - which increases with at least the variables a) number of
> possible features b) number of introspection callbacks c) number of
> Connection interfaces which might or might not be present, roughly
> O(a*b*c).
>
> Ideally, neither the introspection functions nor their callbacks would
> need to know about things they depend on, whether the user has asked
> for them to do thing X, Y or Z, what interfaces the Connection
> supports, and which other introspection functions depend on their
> results. This could be made possible by having the following
> pseudo-things defined for each "introspectable lump" (Core, one
> Feature)
>
> struct Introspectable {
>   bool makesSenseForOffline;
>   bool makesSenseForOnline; // maybe there aren't any which would make
> sense for offline but not online, so maybe this can be omitted and
> considered always true
>   QStringList dependsOnInterfaces;
>   Features dependsOnFeatures; /* everything depends on core, which is
> 0, and core has no dependencies, handled specially */
>   IntrospectFunc startIntrospectionFunction;
> };
> // Feature flag value (0 for core) -> info
> Map<uint, Introspectable> introspectables;
>
> The sets of requested-and-satisfied, requested-but-pending and
> requested-but-found-to-be-unsatisfiable features should be retained, I
> guess. However, the sets should be changed to QSet to properly be able
> to include the core in them. In addition, two new sets should be
> added: features which are currently being introspected, and features
> which have been requested at some point using becomeReady(). However,
> the badly-interacting-with-additional-feature-requests introspect
> queue should go away. So the total data structures become:
>
> QSet<uint> requestedFeatures; // requested at some point, must all be
> in either satisfied or missing to signal being ready for a new state
> QSet<uint> pendingFeatures; // the ones in requested which haven't yet
> been completely introspected, and their (recursive) dependencies
> QSet<uint> inFlightFeatures; // subset of pending - the ones currently
> being introspected - never can include a feature and its dependency at
> the same time
> QSet<uint> satisfiedFeatures; // the features which have been
> satisfied by introspection, or *have no relevant introspection for
> this state* (ie. if you request SelfContact when offline, that is
> "ready" as much as it can be in the offline state without doing
> anything)
> QSet<uint> missingFeatures; // the features which would be relevant
> for this state, but for which introspection has failed, the necessary
> interfaces aren't there, or whatever
>
> Of these, requestedFeatures and satisfiedFeatures are the ones, I
> think, which should be exposed directly to the user (like Contact does
> - Contact::requestedFeatures() and Contact:actualFeatures()). To avoid
> verbosely maintaining these sets in introspection callbacks, something
> like the following internal utility pseudo-function should be used:
>
> // To be called by introspect callbacks completing introspection for a
> particular feature, either successfully or otherwise
> void introspectCompleted(uint feature, bool success) {
>   assert (feature in pendingFeatures);
>   assert (feature in inFlightFeatures);
>   if (success)
>     satisfiedFeatures.insert(feature);
>   else
>     missingFeatures.insert(feature);
>   pendingFeatures.remove(feature);
>   inFlightFeatures.remove(feature);
>   singleShot(iterateIntrospection()); // see later
> }
>
> This would help by only having to establish those "chains" of
> introspect functions only to the level of a single feature (note: this
> is rather similar to the old scheme, where the core was the only
> "feature" and there was a single chain, although that was using the
> queue to have slightly simpler callbacks). Example: a pseudo
> introspection chain for the core functionality would be something
> like:
> void startCoreIntrospection() {
>   connect(GetInterfaces(), gotInterfaces);
> }
>
> void gotInterfaces(interfaces) {
>   if (error) {
>     warning() << error;
>     introspectCompleted(0, false);
>     return;
>   }
>
>   mPriv->interfaces = interfaces;
>   // NOTE: DON'T HAVE TO QUEUE DEPENDENT FEATURE INTROSPECTION HERE!
> NEVER DO THAT! The code complexity of that is around (number of
> possible interfaces) * (number of features) which is not particularly
> nice! The introspectable info allows us to do this in a much simpler
> and generic way.
>
>   if (online)
>     connect(GetSelfHandle(), gotSelfHandle);
>   else
>     introspectCompleted(0, true);
> }
>
> void gotSelfHandle(handle) {
>   if (error) {
>     warning() << error;
>     introspectCompleted(0, false);
>     return;
>   }
>
>   mPriv->selfHandle = handle;
>   introspectCompleted(0, true);
> }
>
> To bring all these little chains together, we need a central iteration function.
>
> void iterateIntrospection()
> {
>   if (not online or offline) {
>     // don't do anything just now to avoid spurious becomeReady finishes
>     return;
>   }
>
>   // check if any pending operations for becomeReady should finish now
> based on their requested features having nothing more than what
> satisfiedFeatures + missingFeatures has
>
>   if ((requestedFeatures - (satisfiedFeatures + missingFeatures)).isEmpty()) {
>     // all requested features satisfied or missing
>     if (pendingStatus != status) {
>       status = pendingStatus;
>       emit statusChanged(status);
>     }
>     return; // nothing to do anymore
>   }
>
>   // update pendingFeatures with the difference of requested and
> satisfied + missing AND THEIR DEPENDENCIES (recursively) (using
> Introspectable::dependsOnFeatures)
>   // take care to flag anything with dependencies in missing, and the
> stuff depending on them, as missing
>
>   // find out which features don't have dependencies that are still pending
>   QSet<uint> readyToIntrospect;
>   foreach (uint feature, pendingFeatures) {
>     if (((introspectables[feature].dependsOnFeatures -
> satisfiedFeatures).isEmpty()) // missing doesn't have to be considered
> here anymore
>       readyToIntrospect.insert(feature);
>   }
>
>   // now readyToIntrospect should contain all the features which have
> all their feature dependencies satisfied
>   foreach (uint feature, readyToIntrospect) {
>     if (feature in inFlightFeatures)
>       continue;
>
>     inFlightFeatures.insert(feature);
>
>     if ((online && !introspectables[feature].makesSenseForOnline) ||
> (!online && !introspectables[feature].makesSenseForOffline)) {
>       // No-op satisfy features for which nothing has to be done in
> the current state
>       introspectCompleted(feature, true);
>       return; // will be called with a single-shot soon again
>     }
>
>     if (feature != 0 && not all of
> introspectables[feature].dependsOnInterfaces in interfaces) {
>       // Core is a dependency for everything, so interfaces are
> introspected - if not all of them are present, the feature can't
> possibly be satisfied
>       introspectCompleted(feature, false);
>       return; // will be called with a single-shot soon again
>     }
>
>     // yes, with the dependency info, we can even parallelize
> introspection of several features at once, reducing total round trip
> time considerably with many independent features!
>     (mPriv->*(introspectables[feature].startIntrospectionFunc))();
>   }
> }
>
> with this, becomeReady becomes just putting the asked-for features to
> requestedFeatures and singleShoting iterateIntrospection, with no need
> to handle all the combinations of <we currently have these features>
> <we never will have these features> <the user asked for these
> features> <the remote object supports these interfaces>, just as the
> case was for the got interfaces callback and other possible callbacks
> satisfying dependencies.
>
> The StatusChanged handler should become something like:
> void onStatusChanged(uint newStatus, uint newStatusReason)
> {
>   if (it's Disconnected) {
>     map reason to an invalidation error and signal invalidation;
>   } else {
>     // IMPORTANT - don't lie to the user that features would be
> introspected for online already, just because they were introspected
> for offline before
>     satisfiedFeatures.clear();
>     missingFeatures.clear();
>     // how to handle inFlightFeatures? see below
>
>     if (it's connected) {
>       pendingStatus = status;
>       singleShot(iterateIntrospection);
>       // the statusChanged will be emitted later on when introspection
> is complete
>     } else {
>       // just emit the statusChanged directly
>     }
>   }
> }
>
> One thing remains to be solved though - how introspection currently in
> flight should cope with the sudden status change? I think this needs
> to be solved on a case-by-case basis - the pre-feature Connection
> coped with this fine due to some simple checks in its introspection
> callbacks, relying on D-Bus message ordering guarantees. Some options
> to experiment with:
>  - Check for pendingStatus != status in introspection CBs
>     - and avoid calling introspectCompleted if it's different (and
> also avoid doing anything further)
>     - more complex logic, but minimal code duplication
>  - Have separate startIntrospectFunctions for offline and online
> (which can be set to the same function of course for things this
> doesn't matter for)
>     - and introspectCompleted taking a parameter to convey whether
> it's offline or online
>     - note that introspect functions for the online state should be
> able to cope with 0% of the corresponding offline introspection done
> and 100% and anything in between
>     - less complex logic, but possibility for more code duplication
>
> I might implement these changes myself, but that would occur during
> the latter half of next week at the earliest - so if you happen to
> find yourself out of work and/or unhappy with how Connection currently
> performs, feel free to try and start experimenting on how implementing
> this would turn out.
>
>   
So I finally had time to take a look at those mails and have some things 
to comment.

First the proposal is really good I just have some points I would like 
to clarify.

Maybe I misunderstood some points here but let me say what I understood, 
feel free to correct me if I am wrong:
- In your proposal, connection "ready" by calling becomeReady will work 
in 2 different situations, when you are offline and when you are online. 
So if the connection is still offline (just created, no requestConnect 
called), and the user calls becomeReady(...) it will finish all 
operations, including for no features (0) at some point. If after that 
the user calls requestConnect it will have to call becomeReady(...) 
again and wait for it to signal finish.
If this is true, as I understood, I would like to propose another solution.

In all other classes isReady/becomeReady should be called just once, and 
when they signal finish, it mean they finished for the whole class, not 
for a state of the class. I would like this to be true for connection 
also, so the proposal is as follow.

Some features may be "ready" for both offline and online, that's why I 
see your point in finishing the operation depending on the state, but 
this could confuse users. This is the case for SimplePresence. Some 
"parts" of simple presence are available for offline and some just when 
you are online, for example availableStatuses is available for offline, 
and selfPresence is only available when you are online, for obvious reasons.

So a solution to this is that, any feature that can be "ready" for both 
cases (offline and online) should be splitted in more than one feature. 
Using SimplePresence as example we could have 
FeatureSimplePresenceAvailableStatuses and FeatureSelfPresence, 
something like this.

So becomeReady(FeatureSimplePresenceAvailableStatuses) should emit 
finish by the time we have the statuses, no matter if we are online or 
offline, and becomeReady(FeatureSelfPresence) should emit finish only 
when we go online and have retrieved all information for self presence.

Examples:

    Connection *conn;

    void connCoreFunctionalityReady(...) {
           // now you can use everything on connection, but the ones 
that depends on features that were not requested and that were requested 
but are not ready yet
    }

    void connFeatureSimplePresenceStatusesFinished(...) {
          // update UI with possible presence statuses to be set
          ...
    }

    void connFeatureSelfPresenceFinished(...) {
          // now you can use the methods to get/set your self presence
          ...
    }

    conn = new Connection(...);
    connect(conn->becomeReady(0), finished, connCoreFunctionalityReady, 
...) // no features  
    connect(conn->becomeReady(FeatureSimplePresenceStatuses), finished, 
connFeatureSimplePresenceStatusesFinished, ...)
    connect(conn->becomeReady(FeatureSelfPresence), finished, 
connFeatureSelfPresenceFinished, ...) // it can be selfContact, just to 
ilustrate

    processEvents();
    ...

    // connection is offline but simple presence statuses are available, 
connFeatureSimplePresenceStatusesFinished is called
   
    conn->requestConnect();
   
    processEvents();
    ....
    // connCoreFunctionalityReady called
    ...
    // connFeaureSelfPresenceFinished called

   
There is a problem with this approach, as in all other classes, the core 
functionality is "ready" before any feature and in the connection case 
it MAY be ready after some feature is already ready. But this at least 
follows one single path where users just have to call becomeReady once, 
and when they signal finish, they can use the class methods regarding 
what features were ready at that time, and this could be documented. We 
also need to define what core means, so this can change.

That's it, as I said, I may be completelly wrong, so feel free to correct me

BR
Andrunko