Attachment 85625 Details for Bug 37381 – Presence: use a sequence number, not a timestamp, to test "most recent"

[patch] Presence: use a sequence number, not a timestamp, to test "most recent"

0002-Presence-use-a-sequence-number-not-a-timestamp-to-te.patch (text/plain), 14.44 KB, created by Simon McVittie on 2013-09-11 12:03:17 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2013-09-11 12:03:17 UTC

Size: 14.44 KB

patch

obsolete

>From d37ec9bd4551e9cfcdb2fb033babff46e0c1f82e Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Wed, 11 Sep 2013 12:39:02 +0100
>Subject: [PATCH 2/3] Presence: use a sequence number, not a timestamp, to test
> "most recent"
>
>This means the client-types test deterministically gets a different
>result: it was previously relying on the PC and the phone coming online
>near-simultaneously, which resulted in the PC "winning" the comparison
>(except when the test ran close to an integer number of
>seconds-since-the-epoch, when it failed).
>
>Now, the phone has a more recent "most recently available"
>sequence number, so it "wins". This might be considered to make
><https://bugs.freedesktop.org/show_bug.cgi?id=32139> regress,
>depending which tie-breaker you consider to be more important.
>
>The change to prefer_higher_priority_resources() illustrates one
>effect of this change: the priority never actually matters
>unless neither of the resources under consideration has ever
>been "available", because the "most recently available"
>sequence number is considered more important.
>
>It's not completely clear whether the "most recently available"
>sequence number is intended to tie-break between resources that are
>away/busy (display the one that went away more recently), or that are
>available (display the one that arrived more recently) - at the
>moment it does both.
>---
> src/conn-presence.c           |  4 +--
> src/connection.c              |  7 +++--
> src/presence-cache.c          |  3 +--
> src/presence.c                | 19 +++++++++-----
> src/presence.h                |  3 +--
> tests/test-presence.c         | 60 +++++++++++++++----------------------------
> tests/twisted/client-types.py |  9 +++----
> 7 files changed, 42 insertions(+), 63 deletions(-)
>
>diff --git a/src/conn-presence.c b/src/conn-presence.c
>index e06062f..394172e 100644
>--- a/src/conn-presence.c
>+++ b/src/conn-presence.c
>@@ -889,7 +889,7 @@ store_shared_statuses (GabbleConnection *self,
>     {
>       /* Update with the remote presence */
>       rv = gabble_presence_update (self->self_presence, resource, presence_id,
>-          status_message, prio, NULL, time (NULL));
>+          status_message, prio, NULL);
>     }
> 
>   g_free (resource);
>@@ -1799,7 +1799,7 @@ set_own_status_cb (GObject *obj,
>     }
> 
>   if (gabble_presence_update (conn->self_presence, resource, i,
>-          message_str, prio, NULL, time (NULL)))
>+          message_str, prio, NULL))
>     {
>       if (tp_base_connection_get_status (base) !=
>               TP_CONNECTION_STATUS_CONNECTED)
>diff --git a/src/connection.c b/src/connection.c
>index 5556b1e..e703e28 100644
>--- a/src/connection.c
>+++ b/src/connection.c
>@@ -544,7 +544,7 @@ gabble_connection_constructed (GObject *object)
>   self->self_presence = gabble_presence_new ();
>   g_assert (priv->resource);
>   gabble_presence_update (self->self_presence, priv->resource,
>-      GABBLE_PRESENCE_AVAILABLE, NULL, priv->priority, NULL, time (NULL));
>+      GABBLE_PRESENCE_AVAILABLE, NULL, priv->priority, NULL);
> }
> 
> static void
>@@ -740,17 +740,16 @@ gabble_connection_set_property (GObject      *object,
>         {
>           gchar *old_resource = priv->resource;
>           gchar *new_resource = g_value_dup_string (value);
>-          time_t now = time (NULL);
> 
>           priv->resource = new_resource;
> 
>           /* Add self presence for new resource... */
>           gabble_presence_update (self->self_presence, new_resource,
>               self->self_presence->status, self->self_presence->status_message,
>-              priv->priority, NULL, now);
>+              priv->priority, NULL);
>           /* ...and remove it for the old one. */
>           gabble_presence_update (self->self_presence, old_resource,
>-              GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL, now);
>+              GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL);
> 
>           g_free (old_resource);
>         }
>diff --git a/src/presence-cache.c b/src/presence-cache.c
>index f7d02ee..b3a0f05 100644
>--- a/src/presence-cache.c
>+++ b/src/presence-cache.c
>@@ -2136,8 +2136,7 @@ gabble_presence_cache_do_update (
>   old_cap_set = gabble_presence_dup_caps (presence);
> 
>   ret = gabble_presence_update (presence, resource, presence_id,
>-      status_message, priority, update_client_types,
>-      time (NULL));
>+      status_message, priority, update_client_types);
> 
>   new_cap_set = gabble_presence_peek_caps (presence);
> 
>diff --git a/src/presence.c b/src/presence.c
>index db9830c..f2b6baa 100644
>--- a/src/presence.c
>+++ b/src/presence.c
>@@ -54,10 +54,11 @@ struct _Resource {
>     GabblePresenceId status;
>     gchar *status_message;
>     gint8 priority;
>-    /* The last time we saw an available (or chatty! \o\ /o/) presence for
>-     * this resource.
>-     */
>-    time_t last_available;
>+    /* The last sequence_number when we saw an available (or chatty! \o\ /o/)
>+     * presence for this resource. It seems OK to assume this won't wrap
>+     * around, since that'd take > 4000 million presence updates in a
>+     * single session. */
>+    guint last_available;
> };
> 
> struct _GabblePresencePrivate {
>@@ -72,6 +73,10 @@ struct _GabblePresencePrivate {
>     guint olpc_views;
> 
>     gchar *active_resource;
>+
>+    /* Used to get a monotonically increasing sequence number for
>+     * last_available. */
>+    guint sequence_number;
> };
> 
> static Resource *
>@@ -144,6 +149,7 @@ gabble_presence_init (GabblePresence *self)
>   priv->data_forms = g_ptr_array_new_with_free_func (
>       (GDestroyNotify) g_object_unref);
>   priv->resources = NULL;
>+  priv->sequence_number = 1;
> 
>   self->status = GABBLE_PRESENCE_UNKNOWN;
> }
>@@ -462,8 +468,7 @@ gabble_presence_update (GabblePresence *presence,
>                         GabblePresenceId status,
>                         const gchar *status_message,
>                         gint8 priority,
>-                        gboolean *update_client_types,
>-                        time_t now)
>+                        gboolean *update_client_types)
> {
>   GabblePresencePrivate *priv = presence->priv;
>   Resource *res;
>@@ -539,7 +544,7 @@ gabble_presence_update (GabblePresence *presence,
>       res->priority = priority;
> 
>       if (res->status >= GABBLE_PRESENCE_AVAILABLE)
>-          res->last_available = now;
>+          res->last_available = (priv->sequence_number++);
>     }
> 
>   /* select the most preferable Resource and update presence->* based on our
>diff --git a/src/presence.h b/src/presence.h
>index e2d5181..c9ec8ea 100644
>--- a/src/presence.h
>+++ b/src/presence.h
>@@ -90,8 +90,7 @@ GabblePresence* gabble_presence_new (void);
> gboolean gabble_presence_update (GabblePresence *presence,
>     const gchar *resource, GabblePresenceId status,
>     const gchar *status_message, gint8 priority,
>-    gboolean *update_client_types,
>-    time_t now);
>+    gboolean *update_client_types);
> 
> void gabble_presence_set_capabilities (GabblePresence *presence,
>     const gchar *resource,
>diff --git a/tests/test-presence.c b/tests/test-presence.c
>index 8319cb2..2d9fdab 100644
>--- a/tests/test-presence.c
>+++ b/tests/test-presence.c
>@@ -19,7 +19,6 @@ big_test_of_doom (void)
>   const gchar *resource;
>   GabblePresence *presence;
>   GabbleCapabilitySet *cap_set;
>-  time_t now = time (NULL);
>   GPtrArray *data_forms = g_ptr_array_new ();
> 
>   /* When we create a new presence, we know nothing about the contact in
>@@ -33,16 +32,16 @@ big_test_of_doom (void)
>    * contact's presence.
>    */
>   g_assert (TRUE == gabble_presence_update (presence, NULL,
>-    GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL, now));
>+    GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL));
>   g_assert (GABBLE_PRESENCE_OFFLINE == presence->status);
>   g_assert (NULL == presence->status_message);
> 
>   /* offline presence from unknown resource: no change */
>   g_assert (FALSE == gabble_presence_update (presence, "foo",
>-    GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL, now));
>+    GABBLE_PRESENCE_OFFLINE, NULL, 0, NULL));
>   /* available presence from unknown resource: change */
>   g_assert (TRUE == gabble_presence_update (presence, "foo",
>-    GABBLE_PRESENCE_AVAILABLE, NULL, 0, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, NULL, 0, NULL));
> 
>   /* accumulated presence has changed; status message unchanged */
>   g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status);
>@@ -50,10 +49,10 @@ big_test_of_doom (void)
> 
>   /* available presence again; no change */
>   g_assert (FALSE == gabble_presence_update (presence, "foo",
>-    GABBLE_PRESENCE_AVAILABLE, NULL, 0, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, NULL, 0, NULL));
>   /* available presence again, but with status message: change */
>   g_assert (TRUE == gabble_presence_update (presence, "foo",
>-    GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL));
> 
>   /* accumulated presence unchanged; status message changed */
>   g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status);
>@@ -61,15 +60,12 @@ big_test_of_doom (void)
> 
>   /* same presence again; no change */
>   g_assert (FALSE == gabble_presence_update (presence, "foo",
>-    GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL, now));
>-
>-  /* time passes */
>-  now++;
>+    GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL));
> 
>   /* presence from different resource, but equal present-ness and equal
>    * status message; unchanged */
>   g_assert (FALSE == gabble_presence_update (presence, "bar",
>-    GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL));
> 
>   g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status);
>   g_assert (0 == strcmp ("status message", presence->status_message));
>@@ -78,49 +74,34 @@ big_test_of_doom (void)
>   g_assert (0 == strcmp ("bar",
>         gabble_presence_pick_resource_by_caps (presence, 0, NULL, NULL)));
> 
>-  /* mountain ranges form */
>-  now++;
>-
>   /* presence from different resource, but equal present-ness and different
>    * status message; changed */
>   g_assert (TRUE == gabble_presence_update (presence, "baz",
>-    GABBLE_PRESENCE_AVAILABLE, "dingbats", 0, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, "dingbats", 0, NULL));
> 
>   g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status);
>   g_assert (0 == strcmp ("dingbats", presence->status_message));
> 
>-  /* babies are born */
>-  now++;
>-
>   /* presence with higher priority; presence and message changed */
>   g_assert (TRUE == gabble_presence_update (presence, "bar",
>-    GABBLE_PRESENCE_AVAILABLE, "dingoes", 1, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, "dingoes", 1, NULL));
> 
>   g_assert (GABBLE_PRESENCE_AVAILABLE == presence->status);
>   g_assert (0 == strcmp ("dingoes", presence->status_message));
> 
>-  /* third-world dictators are deposed */
>-  now++;
>-
>   /* now foo is newer, so the next voip call would prefer that */
>   g_assert (FALSE == gabble_presence_update (presence, "foo",
>-    GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, "status message", 0, NULL));
>   g_assert (0 == strcmp ("foo",
>         gabble_presence_pick_resource_by_caps (presence, 0, NULL, NULL)));
> 
>-  /* portal 2 is released */
>-  now++;
>-
>   /* presence from first resource with greated present-ness: change */
>   g_assert (TRUE == gabble_presence_update (presence, "foo",
>-    GABBLE_PRESENCE_CHAT, "status message", 0, NULL, now));
>-
>-  /* seasons change */
>-  now++;
>+    GABBLE_PRESENCE_CHAT, "status message", 0, NULL));
> 
>   /* make bar be the latest presence: no change, since foo is more present */
>   g_assert (FALSE == gabble_presence_update (presence, "bar",
>-    GABBLE_PRESENCE_AVAILABLE, "dingoes", 1, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, "dingoes", 1, NULL));
> 
>   /* we still prefer foo for the voip calls, because it's more present */
>   g_assert (0 == strcmp ("foo",
>@@ -136,7 +117,7 @@ big_test_of_doom (void)
> 
>   /* give voice cap to second resource, but make priority negative */
>   g_assert (FALSE == gabble_presence_update (presence, "bar",
>-    GABBLE_PRESENCE_AVAILABLE, "dingoes", -1, NULL, now));
>+    GABBLE_PRESENCE_AVAILABLE, "dingoes", -1, NULL));
>   cap_set = gabble_capability_set_new ();
>   gabble_capability_set_add (cap_set, NS_GOOGLE_FEAT_VOICE);
>   gabble_presence_set_capabilities (presence, "bar", cap_set, data_forms, 0);
>@@ -161,7 +142,7 @@ big_test_of_doom (void)
>   /* presence turns up from null resource; it trumps other presence regardless
>    * of whether status is more present or not */
>   g_assert (TRUE == gabble_presence_update (presence, NULL,
>-    GABBLE_PRESENCE_OFFLINE, "gone", 0, NULL, now));
>+    GABBLE_PRESENCE_OFFLINE, "gone", 0, NULL));
>   g_assert (GABBLE_PRESENCE_OFFLINE == presence->status);
>   g_assert (0 == strcmp ("gone", presence->status_message));
> 
>@@ -187,15 +168,14 @@ static void
> prefer_higher_priority_resources (void)
> {
>   GabblePresence *presence = gabble_presence_new ();
>-  time_t now = time (NULL);
> 
>-  /* 'foo' and 'bar' are equally available, at the same time, but bar has a
>-   * lower priority.
>+  /* 'foo' and 'bar' are equally available, and were last available
>+   * at the same time (i.e. never), but bar has a lower priority.
>    */
>-  gabble_presence_update (presence, "foo", GABBLE_PRESENCE_AVAILABLE, "foo", 10,
>-      NULL, now);
>-  gabble_presence_update (presence, "bar", GABBLE_PRESENCE_AVAILABLE, "bar", 5,
>-      NULL, now);
>+  gabble_presence_update (presence, "foo", GABBLE_PRESENCE_AWAY, "foo", 10,
>+      NULL);
>+  gabble_presence_update (presence, "bar", GABBLE_PRESENCE_AWAY, "bar", 5,
>+      NULL);
> 
>   /* We should be sure to prefer "foo"'s status message to "bar"'s.
>    */
>diff --git a/tests/twisted/client-types.py b/tests/twisted/client-types.py
>index a1c698b..6e56106 100644
>--- a/tests/twisted/client-types.py
>+++ b/tests/twisted/client-types.py
>@@ -214,12 +214,12 @@ def test2(q, bus, conn, stream):
>                                 dbus_interface=cs.CONN_IFACE_CLIENT_TYPES)
>     assertSameSets(['pc'], types[handle])
> 
>-    # phone comes online
>+    # Phone comes online. It "was available more recently" so it wins.
>     contact_online(q, conn, stream, marco_phone, PHONE, initial=False)
> 
>     types = conn.GetClientTypes([handle],
>                                 dbus_interface=cs.CONN_IFACE_CLIENT_TYPES)
>-    assertSameSets(['pc'], types[handle])
>+    assertSameSets(['phone'], types[handle])
> 
>     sync_stream(q, stream)
> 
>@@ -228,10 +228,7 @@ def test2(q, bus, conn, stream):
> 
>     # no presence signal
> 
>-    q.expect('dbus-signal', signal='ClientTypesUpdated',
>-             args=[handle, ['phone']])
>-
>-    # pidgin comes back online
>+    # Pidgin comes back online. Now it's the more recently available.
>     caps, _, _ = build_stuff(PC)
>     stream.send(make_presence(marco_pidgin, status='hello', caps=caps))
> 
>-- 
>1.8.4.rc3
>

Actions: View | Splinter Review

Attachments on bug 37381: 56982 | 85624 | 85625

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.