[Bug 37381] client-types.py appears to have a race

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=37381

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jonny.lamb at collabora.co.uk,
                   |                            |sjoerd at luon.net

--- Comment #6 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Cc Jonny and Sjoerd, contributors to Bug #32139, in the hope that they can shed
some light on why the algorithm is the way it is...

The limited resolution of last_available is irritating, but we can fix that by
making it a sequence number rather than a timestamp.

(In reply to comment #0)
> If there are multiple clients with the same availability, but different
> client types, then Gabble seems to (sometimes?, always?) use phone as the
> client type.

It picks the "best" resource, and uses its status, status message and client
type. That seems right - the problem is the algorithm for "best", which is
pretty odd.

The big messy conditional looks wrong, or at least non-deterministic:

      if (best == NULL ||
          r->status > best->status ||
          (r->status == best->status &&
              (r->last_available > best->last_available ||
               r->priority > best->priority)) ||
          (r->client_type & GABBLE_CLIENT_TYPE_PC
              && !(best->client_type & GABBLE_CLIENT_TYPE_PC)))
        best = r;

I'd feel happier about this if it was expressed in the form of a strcmp()-style
"is x better than y?" function. In addition to the info in the struct itself,
comparison currently depends on an implicit extra piece of info: when a
resource is added (goes from unavailable to any online status), it goes to the
end of the list; when a resource is updated (goes from any online status to any
other online status), it doesn't. If the comparison depending on that is
intentional, I'd prefer it to be explicit.

As a result, this:

    - phone and Empathy both offline
    - phone becomes Available
      - phone is selected
    - Empathy becomes Busy
      - Empathy is selected, because its client type is "better"

does not lead to the same result as this:

    - phone and Empathy both offline
    - Empathy becomes Busy
      - Empathy is selected
    - phone becomes Available
      - phone is selected, because its status is "better"

Adding a couple of extra parameters to resource_better_than() could let us use
it for both "which resource should I use for chat/calls/FT?" and "which
resource should be displayed in the contact list?", with different rules if
necessary - but we'd need to agree on what those rules were. Is "PC beats
phone" a last-resort tie-breaker, or top priority, for instance?

Things that seem reasonable as a basis for comparison to me include:

* something is better than nothing (unless it has negative priority and we're
trying to communicate with it)
* PCs are better (for status display) than phones, according to Bug #32139
* phones are better (for calls) than PCs
* high status (available) is better than low status (away)
* if both are available, the one that transitioned more recently is better
* if both are away/busy, the one that was available more recently is better
* high priority is better than low priority
* older connection is better than newer connection? (or the other way round?)

but the question is how to deal with each pair "x1 > x2 && y1 < y2". The bullet
points there match how resource_better_than() behaves (first = most important).

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.