[Telepathy] TelepathyQt4: State of the Connection

[Olli Salli]

> 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.

-- 

Br,
Olli Salli