GNOME.org
 

[gnome-online-accounts] identity: dramatically simplify alarm logic

commit b2f1b8a70f45848e0d7b88c9c56790a746dd5802
Author: Ray Strode <rstrode redhat com>
Date:   Thu May 8 13:31:16 2014 -0400

    identity: dramatically simplify alarm logic
    
    The GoaAlarm code is unwieldy. Primarily this is because
    it supports resetting the alarm after creation, but can't
    clean up old state right away due to a bug in GLib
    (bug 705395). All this complexitly is leading to bugs, and
    caos.
    
    This commit introduces some zen to the situation by making
    GoaAlarm immutable and much simpler. Now to reset an alarm,
    the identity code just instantiates a new one and destroys the
    old one.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=729718

 src/goaidentity/goaalarm.c            |  177 ++++-----------------------------
 src/goaidentity/goaalarm.h            |    5 +-
 src/goaidentity/goakerberosidentity.c |  131 ++++++++++++-------------
 3 files changed, 82 insertions(+), 231 deletions(-)
---
diff --git a/src/goaidentity/goaalarm.c b/src/goaidentity/goaalarm.c
index dbe66f1..7085229 100644
--- a/src/goaidentity/goaalarm.c
+++ b/src/goaidentity/goaalarm.c
@@ -43,8 +43,6 @@ typedef enum
 
 struct _GoaAlarmPrivate
 {
-  GCancellable *cancellable;
-  gulong cancelled_id;
   GDateTime *time;
   GDateTime *previous_wakeup_time;
   GMainContext *context;
@@ -71,60 +69,23 @@ enum
 
 static void schedule_wakeups (GoaAlarm *self);
 static void schedule_wakeups_with_timeout_source (GoaAlarm *self);
+static void goa_alarm_set_time (GoaAlarm *self, GDateTime *time);
+static void clear_wakeup_source_pointer (GoaAlarm *self);
 static guint signals[NUMBER_OF_SIGNALS] = { 0 };
 
 G_DEFINE_TYPE (GoaAlarm, goa_alarm, G_TYPE_OBJECT);
 
 static void
-clear_scheduled_immediate_wakeup (GoaAlarm *self)
-{
-  g_clear_pointer (&self->priv->immediate_wakeup_source,
-                   (GDestroyNotify) g_source_destroy);
-}
-
-static void
-clear_scheduled_wakeups (GoaAlarm *self, GSource *source, GInputStream *stream)
-{
-  g_rec_mutex_lock (&self->priv->lock);
-  clear_scheduled_immediate_wakeup (self);
-
-  if (self->priv->type != GOA_ALARM_TYPE_UNSCHEDULED)
-    {
-      g_source_destroy (source);
-
-      if (stream != NULL)
-        {
-          GError *error;
-          gboolean is_closed;
-
-          error = NULL;
-          is_closed = g_input_stream_close (stream, NULL, &error);
-
-          if (!is_closed)
-            {
-              g_warning ("GoaAlarm: could not close timer stream: %s", error->message);
-              g_error_free (error);
-            }
-
-          g_object_unref (stream);
-        }
-    }
-
-  g_clear_pointer (&self->priv->previous_wakeup_time,
-                   (GDestroyNotify) g_date_time_unref);
-
-  self->priv->type = GOA_ALARM_TYPE_UNSCHEDULED;
-  g_rec_mutex_unlock (&self->priv->lock);
-}
-
-static void
 goa_alarm_dispose (GObject *object)
 {
   GoaAlarm *self = GOA_ALARM (object);
 
-  g_clear_object (&self->priv->cancellable);
+  g_clear_object (&self->priv->stream);
+  g_clear_pointer (&self->priv->immediate_wakeup_source, (GDestroyNotify) g_source_destroy);
+  g_clear_pointer (&self->priv->scheduled_wakeup_source, (GDestroyNotify) g_source_destroy);
   g_clear_pointer (&self->priv->context, (GDestroyNotify) g_main_context_unref);
   g_clear_pointer (&self->priv->time, (GDestroyNotify) g_date_time_unref);
+  g_clear_pointer (&self->priv->previous_wakeup_time, (GDestroyNotify) g_date_time_unref);
 
   G_OBJECT_CLASS (goa_alarm_parent_class)->dispose (object);
 }
@@ -134,8 +95,6 @@ goa_alarm_finalize (GObject *object)
 {
   GoaAlarm *self = GOA_ALARM (object);
 
-  clear_scheduled_wakeups (self, self->priv->scheduled_wakeup_source, self->priv->stream);
-
   g_rec_mutex_clear (&self->priv->lock);
 
   G_OBJECT_CLASS (goa_alarm_parent_class)->finalize (object);
@@ -154,7 +113,7 @@ goa_alarm_set_property (GObject      *object,
     {
     case PROP_TIME:
       time = (GDateTime *) g_value_get_boxed (value);
-      goa_alarm_set_time (self, time, self->priv->cancellable);
+      goa_alarm_set_time (self, time);
       break;
     default:
       G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, param_spec);
@@ -218,54 +177,9 @@ static void
 goa_alarm_init (GoaAlarm *self)
 {
   self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, GOA_TYPE_ALARM, GoaAlarmPrivate);
-  self->priv->type = GOA_ALARM_TYPE_UNSCHEDULED;
   g_rec_mutex_init (&self->priv->lock);
 }
 
-static gboolean
-async_alarm_cancel_idle_cb (gpointer user_data)
-{
-  GoaAlarm *self;
-  GInputStream *stream;
-  GSource *source;
-  GTask *task = G_TASK (user_data);
-  gpointer task_data;
-
-  self = g_task_get_source_object (task);
-  source = (GSource *) g_object_get_data (G_OBJECT (task), "alarm-scheduled-wakeup-source");
-  task_data = g_object_get_data (G_OBJECT (task), "alarm-stream");
-  stream = (task_data == NULL) ? NULL : G_INPUT_STREAM (task_data);
-
-  clear_scheduled_wakeups (self, source, stream);
-  return G_SOURCE_REMOVE;
-}
-
-static void
-on_cancelled (GCancellable *cancellable, gpointer user_data)
-{
-  GoaAlarm *self = GOA_ALARM (user_data);
-  GSource *idle_source;
-  GTask *task;
-
-  task = g_task_new (self, NULL, NULL, NULL);
-
-  g_object_set_data_full (G_OBJECT (task),
-                          "alarm-scheduled-wakeup-source",
-                          g_source_ref (self->priv->scheduled_wakeup_source),
-                          (GDestroyNotify) g_source_unref);
-
-  if (self->priv->stream != NULL)
-    g_object_set_data_full (G_OBJECT (task), "alarm-stream", g_object_ref (self->priv->stream), 
g_object_unref);
-
-  idle_source = g_idle_source_new ();
-  g_source_set_priority (idle_source, G_PRIORITY_HIGH_IDLE);
-  g_source_set_callback (idle_source, async_alarm_cancel_idle_cb, g_object_ref (task), g_object_unref);
-  g_source_attach (idle_source, self->priv->context);
-  g_source_unref (idle_source);
-
-  g_object_unref (task);
-}
-
 static void
 fire_alarm (GoaAlarm *self)
 {
@@ -334,42 +248,25 @@ on_immediate_wakeup_source_ready (GoaAlarm *self)
   g_return_val_if_fail (self->priv->type != GOA_ALARM_TYPE_UNSCHEDULED, FALSE);
 
   g_rec_mutex_lock (&self->priv->lock);
-  if (g_cancellable_is_cancelled (self->priv->cancellable))
-    goto out;
-
   fire_or_rearm_alarm (self);
-
-out:
   g_rec_mutex_unlock (&self->priv->lock);
   return FALSE;
 }
 
 #ifdef HAVE_TIMERFD
 static gboolean
-on_timer_source_ready (GObject *stream, GTask *task)
+on_timer_source_ready (GObject *stream, GoaAlarm *self)
 {
   gint64 number_of_fires;
   gssize bytes_read;
   gboolean run_again = FALSE;
   GError *error = NULL;
-  GoaAlarm *self;
-  GCancellable *cancellable;
-
-  self = g_task_get_source_object (task);
-  cancellable = g_task_get_cancellable (task);
 
   g_return_val_if_fail (GOA_IS_ALARM (self), FALSE);
 
   g_rec_mutex_lock (&self->priv->lock);
 
-  if (self->priv->type == GOA_ALARM_TYPE_UNSCHEDULED)
-    {
-      g_debug ("GoaAlarm: timer source was unscheduled after "
-               "callback was invoked, but before callback got "
-               "the lock.");
-      goto out;
-    }
-  else if (self->priv->type != GOA_ALARM_TYPE_TIMER)
+  if (self->priv->type != GOA_ALARM_TYPE_TIMER)
     {
       g_warning ("GoaAlarm: timer source ready callback called "
                  "when timer source isn't supposed to be used. "
@@ -377,9 +274,6 @@ on_timer_source_ready (GObject *stream, GTask *task)
       goto out;
     }
 
-  if (g_cancellable_is_cancelled (cancellable))
-    goto out;
-
   bytes_read =
     g_pollable_input_stream_read_nonblocking (G_POLLABLE_INPUT_STREAM (stream),
                                               &number_of_fires, sizeof (gint64),
@@ -418,7 +312,6 @@ schedule_wakeups_with_timerfd (GoaAlarm *self)
   int fd;
   int result;
   GSource *source;
-  GTask *task;
   static gboolean seen_before = FALSE;
 
   if (!seen_before)
@@ -451,16 +344,14 @@ schedule_wakeups_with_timerfd (GoaAlarm *self)
   self->priv->type = GOA_ALARM_TYPE_TIMER;
   self->priv->stream = g_unix_input_stream_new (fd, TRUE);
 
-  task = g_task_new (self, self->priv->cancellable, NULL, NULL);
-
   source =
     g_pollable_input_stream_create_source (G_POLLABLE_INPUT_STREAM
                                            (self->priv->stream),
-                                           self->priv->cancellable);
+                                           NULL);
   self->priv->scheduled_wakeup_source = source;
   g_source_set_callback (self->priv->scheduled_wakeup_source,
-                         (GSourceFunc) on_timer_source_ready, task,
-                         (GDestroyNotify) g_object_unref);
+                         (GSourceFunc) on_timer_source_ready, self,
+                         (GDestroyNotify) clear_wakeup_source_pointer);
   g_source_attach (self->priv->scheduled_wakeup_source, self->priv->context);
   g_source_unref (source);
 
@@ -478,15 +369,11 @@ on_timeout_source_ready (GoaAlarm *self)
 
   g_rec_mutex_lock (&self->priv->lock);
 
-  if (g_cancellable_is_cancelled (self->priv->cancellable) ||
-      self->priv->type == GOA_ALARM_TYPE_UNSCHEDULED)
+  if (self->priv->type == GOA_ALARM_TYPE_UNSCHEDULED)
     goto out;
 
   fire_or_rearm_alarm (self);
 
-  if (g_cancellable_is_cancelled (self->priv->cancellable))
-    goto out;
-
   schedule_wakeups_with_timeout_source (self);
 
 out:
@@ -495,7 +382,7 @@ out:
 }
 
 static void
-clear_timeout_source_pointer (GoaAlarm *self)
+clear_wakeup_source_pointer (GoaAlarm *self)
 {
   self->priv->scheduled_wakeup_source = NULL;
 }
@@ -529,7 +416,7 @@ schedule_wakeups_with_timeout_source (GoaAlarm *self)
   g_source_set_callback (self->priv->scheduled_wakeup_source,
                          (GSourceFunc)
                          on_timeout_source_ready,
-                         self, (GDestroyNotify) clear_timeout_source_pointer);
+                         self, (GDestroyNotify) clear_wakeup_source_pointer);
 
   g_source_attach (self->priv->scheduled_wakeup_source, self->priv->context);
   g_source_unref (source);
@@ -579,38 +466,12 @@ schedule_immediate_wakeup (GoaAlarm *self)
   g_source_unref (source);
 }
 
-void
-goa_alarm_set_time (GoaAlarm *self, GDateTime *time, GCancellable *cancellable)
+static void
+goa_alarm_set_time (GoaAlarm *self, GDateTime *time)
 {
-  if (g_cancellable_is_cancelled (cancellable))
-    return;
-
   g_rec_mutex_lock (&self->priv->lock);
-  if (self->priv->cancellable != NULL && self->priv->cancellable != cancellable)
-    g_cancellable_cancel (self->priv->cancellable);
-
-  if (cancellable != NULL)
-    g_object_ref (cancellable);
-
-  if (self->priv->cancelled_id != 0)
-    g_cancellable_disconnect (self->priv->cancellable, self->priv->cancelled_id);
-
-  g_clear_object (&self->priv->cancellable);
-
-  if (cancellable != NULL)
-    self->priv->cancellable = cancellable;
-  else
-    self->priv->cancellable = g_cancellable_new ();
-
-  self->priv->cancelled_id = g_cancellable_connect (self->priv->cancellable,
-                                                    G_CALLBACK (on_cancelled),
-                                                    self, NULL);
 
   g_date_time_ref (time);
-
-  if (self->priv->time != NULL)
-    g_date_time_unref (self->priv->time);
-
   self->priv->time = time;
 
   if (self->priv->context == NULL)
@@ -631,11 +492,11 @@ goa_alarm_get_time (GoaAlarm *self)
 }
 
 GoaAlarm *
-goa_alarm_new (void)
+goa_alarm_new (GDateTime *alarm_time)
 {
   GoaAlarm *self;
 
-  self = GOA_ALARM (g_object_new (GOA_TYPE_ALARM, NULL));
+  self = GOA_ALARM (g_object_new (GOA_TYPE_ALARM, "time", alarm_time, NULL));
 
   return GOA_ALARM (self);
 }
diff --git a/src/goaidentity/goaalarm.h b/src/goaidentity/goaalarm.h
index 7ef4bde..5f15033 100644
--- a/src/goaidentity/goaalarm.h
+++ b/src/goaidentity/goaalarm.h
@@ -51,10 +51,7 @@ struct _GoaAlarmClass
 
 GType goa_alarm_get_type (void);
 
-GoaAlarm *goa_alarm_new (void);
-void goa_alarm_set_time (GoaAlarm     *alarm,
-                         GDateTime    *time,
-                         GCancellable *cancellable);
+GoaAlarm *goa_alarm_new (GDateTime *time);
 GDateTime *goa_alarm_get_time (GoaAlarm *alarm);
 G_END_DECLS
 #endif /* __GOA_ALARM_H__ */
diff --git a/src/goaidentity/goakerberosidentity.c b/src/goaidentity/goakerberosidentity.c
index 1596a5f..ab86d36 100644
--- a/src/goaidentity/goakerberosidentity.c
+++ b/src/goaidentity/goakerberosidentity.c
@@ -51,13 +51,8 @@ struct _GoaKerberosIdentityPrivate
   guint          expiration_time_idle_id;
 
   GoaAlarm     *expiration_alarm;
-  GCancellable *expiration_alarm_cancellable;
-
   GoaAlarm     *expiring_alarm;
-  GCancellable *expiring_alarm_cancellable;
-
   GoaAlarm     *renewal_alarm;
-  GCancellable *renewal_alarm_cancellable;
 
   VerificationLevel cached_verification_level;
   guint             is_signed_in_idle_id;
@@ -110,8 +105,6 @@ goa_kerberos_identity_dispose (GObject *object)
   GoaKerberosIdentity *self = GOA_KERBEROS_IDENTITY (object);
 
   G_LOCK (identity_lock);
-  clear_alarms (self);
-
   g_clear_object (&self->priv->renewal_alarm);
   g_clear_object (&self->priv->expiring_alarm);
   g_clear_object (&self->priv->expiration_alarm);
@@ -301,9 +294,6 @@ goa_kerberos_identity_init (GoaKerberosIdentity *self)
   self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self,
                                             GOA_TYPE_KERBEROS_IDENTITY,
                                             GoaKerberosIdentityPrivate);
-  self->priv->expiration_alarm = goa_alarm_new ();
-  self->priv->expiring_alarm = goa_alarm_new ();
-  self->priv->renewal_alarm = goa_alarm_new ();
 }
 
 static void
@@ -736,8 +726,6 @@ on_renewal_alarm_fired (GoaAlarm            *alarm,
   g_return_if_fail (GOA_IS_ALARM (alarm));
   g_return_if_fail (GOA_IS_KERBEROS_IDENTITY (self));
 
-  g_clear_object (&self->priv->renewal_alarm_cancellable);
-
   if (self->priv->cached_verification_level == VERIFICATION_LEVEL_SIGNED_IN)
     {
       g_debug ("GoaKerberosIdentity: renewal alarm fired for signed-in identity");
@@ -762,8 +750,6 @@ on_expiring_alarm_fired (GoaAlarm            *alarm,
   g_return_if_fail (GOA_IS_ALARM (alarm));
   g_return_if_fail (GOA_IS_KERBEROS_IDENTITY (self));
 
-  g_clear_object (&self->priv->expiring_alarm_cancellable);
-
   if (self->priv->cached_verification_level == VERIFICATION_LEVEL_SIGNED_IN)
     {
       g_debug ("GoaKerberosIdentity: expiring alarm fired for signed-in identity");
@@ -771,25 +757,38 @@ on_expiring_alarm_fired (GoaAlarm            *alarm,
     }
 }
 
+static gboolean
+unref_alarm (GoaAlarm *alarm)
+{
+  g_object_unref (G_OBJECT (alarm));
+  return G_SOURCE_REMOVE;
+}
+
 static void
-set_alarm (GoaKerberosIdentity  *self,
-           GoaAlarm             *alarm,
-           GDateTime            *alarm_time,
-           GCancellable        **cancellable)
+clear_alarm_and_unref_on_idle (GoaKerberosIdentity  *self,
+                     GoaAlarm            **alarm)
 {
-  GDateTime *old_alarm_time;
+  if (!*alarm)
+    return;
+
+  g_idle_add ((GSourceFunc) unref_alarm, *alarm);
+  *alarm = NULL;
+}
+
+static void
+reset_alarm (GoaKerberosIdentity  *self,
+             GoaAlarm            **alarm,
+             GDateTime            *alarm_time)
+{
+  GDateTime *old_alarm_time = NULL;
 
   G_LOCK (identity_lock);
-  old_alarm_time = goa_alarm_get_time (alarm);
+  if (*alarm)
+    old_alarm_time = goa_alarm_get_time (*alarm);
   if (old_alarm_time == NULL || !g_date_time_equal (alarm_time, old_alarm_time))
     {
-      GCancellable *new_cancellable;
-
-      new_cancellable = g_cancellable_new ();
-      goa_alarm_set_time (alarm, alarm_time, new_cancellable);
-
-      g_clear_object (cancellable);
-      *cancellable = new_cancellable;
+      clear_alarm_and_unref_on_idle (self, alarm);
+      *alarm = goa_alarm_new (alarm_time);
     }
   G_UNLOCK (identity_lock);
 
@@ -798,24 +797,35 @@ set_alarm (GoaKerberosIdentity  *self,
 static void
 disconnect_alarm_signals (GoaKerberosIdentity *self)
 {
-  g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->renewal_alarm),
-                                        G_CALLBACK (on_renewal_alarm_fired),
-                                        self);
-  g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->renewal_alarm),
-                                        G_CALLBACK (on_renewal_alarm_rearmed),
-                                        self);
-  g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->expiring_alarm),
-                                        G_CALLBACK (on_expiring_alarm_fired),
-                                        self);
-  g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->expiration_alarm),
-                                        G_CALLBACK (on_expiration_alarm_rearmed),
-                                        self);
-  g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->expiration_alarm),
-                                        G_CALLBACK (on_expiration_alarm_fired),
-                                        self);
-  g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->expiring_alarm),
-                                        G_CALLBACK (on_expiring_alarm_rearmed),
-                                        self);
+  if (self->priv->renewal_alarm)
+    {
+      g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->renewal_alarm),
+                                            G_CALLBACK (on_renewal_alarm_fired),
+                                            self);
+      g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->renewal_alarm),
+                                            G_CALLBACK (on_renewal_alarm_rearmed),
+                                            self);
+    }
+
+  if (self->priv->expiring_alarm)
+    {
+      g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->expiring_alarm),
+                                            G_CALLBACK (on_expiring_alarm_fired),
+                                            self);
+      g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->expiring_alarm),
+                                            G_CALLBACK (on_expiring_alarm_rearmed),
+                                            self);
+    }
+
+  if (self->priv->expiration_alarm)
+    {
+      g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->expiration_alarm),
+                                            G_CALLBACK (on_expiration_alarm_rearmed),
+                                            self);
+      g_signal_handlers_disconnect_by_func (G_OBJECT (self->priv->expiration_alarm),
+                                            G_CALLBACK (on_expiration_alarm_fired),
+                                            self);
+    }
 }
 
 static void
@@ -872,15 +882,9 @@ reset_alarms (GoaKerberosIdentity *self)
 
   disconnect_alarm_signals (self);
 
-  set_alarm (self,
-             self->priv->renewal_alarm,
-             renewal_time, &self->priv->renewal_alarm_cancellable);
-  set_alarm (self,
-             self->priv->expiring_alarm,
-             expiring_time, &self->priv->expiring_alarm_cancellable);
-  set_alarm (self,
-             self->priv->expiration_alarm,
-             expiration_time, &self->priv->expiration_alarm_cancellable);
+  reset_alarm (self, &self->priv->renewal_alarm, renewal_time);
+  reset_alarm (self, &self->priv->expiring_alarm, expiring_time);
+  reset_alarm (self, &self->priv->expiration_alarm, expiration_time);
 
   g_date_time_unref (renewal_time);
   g_date_time_unref (expiring_time);
@@ -889,23 +893,12 @@ reset_alarms (GoaKerberosIdentity *self)
 }
 
 static void
-cancel_and_clear_cancellable (GCancellable **cancellable)
-{
-  if (cancellable == NULL)
-    return;
-
-  if (!g_cancellable_is_cancelled (*cancellable))
-    g_cancellable_cancel (*cancellable);
-
-  g_clear_object (cancellable);
-}
-
-static void
 clear_alarms (GoaKerberosIdentity *self)
 {
-  cancel_and_clear_cancellable (&self->priv->renewal_alarm_cancellable);
-  cancel_and_clear_cancellable (&self->priv->expiring_alarm_cancellable);
-  cancel_and_clear_cancellable (&self->priv->expiration_alarm_cancellable);
+  disconnect_alarm_signals (self);
+  clear_alarm_and_unref_on_idle (self, &self->priv->renewal_alarm);
+  clear_alarm_and_unref_on_idle (self, &self->priv->expiring_alarm);
+  clear_alarm_and_unref_on_idle (self, &self->priv->expiration_alarm);
 }
 
 static gboolean
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]