cashew: defer all D-Bus signal processing to main loop

src/property_changed_handler.cc

Index: src/property_changed_handler.cc
diff --git a/src/property_changed_handler.cc b/src/property_changed_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7f15ea6d992cf9621a0da56f457479450953dd8c
--- /dev/null
+++ b/src/property_changed_handler.cc
@@ -0,0 +1,73 @@
+// Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/property_changed_handler.h"
+
+#include <glog/logging.h>
+
+namespace cashew {
+
+PropertyChangedHandler::PropertyChangedHandler()
+    : delegate_(NULL), source_id_(0) {}
+
+PropertyChangedHandler::~PropertyChangedHandler() {
+  DeletePendingSignals();
+  if (source_id_ != 0 && !g_source_remove(source_id_)) {
+    LOG(WARNING) << "dtor: g_source_remove failed";
+  }
+}
+
+void PropertyChangedHandler::delegate(PropertyChangedDelegate *delegate) {
+  delegate_ = delegate;
+}
+
+void PropertyChangedHandler::EnqueueSignal(
+    const PropertyChangedSignal& signal) {
+  signal_queue_.push(signal);
+  // schedule glib callback to do deferred signal processing if one is not
+  // already scheduled
+  if (source_id_ != 0) {
+    return;
+  }
+  source_id_ = g_idle_add(StaticOnPropertyChangedCallback, this);

[Daniel Kurtz, 2010/11/29 05:20:17]

I don't know how well this will work.  Mucking with the mainloop asynchronously
seems like a bad idea.
See below.

[Vince Laviano, 2010/11/29 20:13:08]

This is no different than the approach taken in my previous CL and with other
timers in the codebase such as the GetProperties timer. I think that the word
"signal" may be creating some confusion. See my comments below.

[Daniel Kurtz, 2010/11/29 21:52:08]

OK I agree since it is all single-threaded.

+  if (source_id_ == 0) {
+    LOG(ERROR) << "EnqueueSignal: g_idle_add failed";
+    return;
+  }
+}
+
+// Private methods
+
+// static
+gboolean PropertyChangedHandler::StaticOnPropertyChangedCallback(
+    gpointer data) {
+  PropertyChangedHandler *handler =
+      reinterpret_cast<PropertyChangedHandler*>(data);
+  DCHECK(handler != NULL);
+  DCHECK_NE(handler->source_id_, 0);
+  // TODO(vlaviano): we could impose a limit on the number of signals that we're
+  // willing to process per invocation and return TRUE to reschedule this
+  // callback if there are signals remaining in the queue
+  while (!handler->signal_queue_.empty()) {
+    PropertyChangedSignal signal = handler->signal_queue_.front();
+    const std::string& property_name = signal.first;
+    const DBus::Variant& new_value = signal.second;
+    if (handler->delegate_ != NULL) {
+      handler->delegate_->OnPropertyChanged(handler, property_name, new_value);
+    }
+    handler->signal_queue_.pop();
+  }
+  // we don't want to be run again automatically
+  // EnqueueSignal will schedule us as needed
+  handler->source_id_ = 0;

[Daniel Kurtz, 2010/11/29 05:20:17]

There is a race condition if ::EnqueSignal() is called (due to inbound DBus
signal) while ::StaticOnPropertyChangedCallback() is executing.
The just-added idle handler would be immediately cleared, causing DCHECK fail in
at next invokation of this function.
I don't think tracking source_id_ serves any real purpose, so it would be
simpler and less race-prone if you just got rid of source_id_ tracking
everywhere.  Just always do exactly one g_idle_add() on each ::EnqueueSignal(), 
PropChangeCallback() will always do the right thing and drain the queue: whether
it has 1 element (the normal case), >1 element (multiple got enqueued between
PCC calls), or 0 elements (no signals since previous call has drained multiple
enqueued signals).

I don't know enough about GLib to know if this is true or not:
One use of source_id_ could be to keep a list of 'scheduled callbacks that have
not yet been called' (i.e. pending events), so that you could clean up properly
in case of program termination. (i.e.  like in "DeletePendingSignals").

[Vince Laviano, 2010/11/29 20:13:08]

cashew is a single-threaded application structured around a single glib main
event loop, so there's no race condition because there is no way for
EnqueueSignal (or anything other than a real signal handler, see below) to be
called while this method is executing.  

The term "D-Bus signal", and my use of the word "signal" in the names of these
objects and methods, may be creating some confusion. It's not really a Unix
signal (which *could* run simultaneously with other code and cause a race). It's
called a signal in D-Bus-speak to characterize the asynchronous and
unidirectional nature of the communication and to distinguish it from a method
request or method reply, but it's just a callback from the glib main loop
triggered by the receipt of a msg on a D-Bus-related file descriptor. Given the
way the app is structured, only one glib callback can be running at a time.
The purpose of keeping track of the glib source is so that we can remove the
source prior to its being dispatched if the associated object is deleted. 
Otherwise, the source will be passed a this ptr that points to a freed object,
which it will attempt to dereference with unpleasant results.

Also, with the current approach, we only create a source on receiving a D-Bus
signal if one is not already created, so we only have a single source id to
track and remove, rather than a list of scheduled callbacks.

[Daniel Kurtz, 2010/11/29 21:52:08]

OK I agree since it is all single-threaded.

+  return FALSE;

[Daniel Kurtz, 2010/11/29 05:20:17]

http://library.gnome.org/devel/glib/unstable/glib-The-Main-Event-Loop.html

Maybe you could rewrite this as a custom GSource for the GLib main loop.
'->prepare()' could check !signal_queue.empty, and '->dispatch()' would call
delegate_->OnPropertyChanged().

[Vince Laviano, 2010/11/29 20:13:08]

I could, but I think that a simple g_idle_add meets our needs here. I'm
concerned that with the proposed approach we'd be adding the complexity of
GSourceFuncs without deriving any benefit from their increased flexibility.

[Daniel Kurtz, 2010/11/29 21:52:08]

OK I agree, if creating a new idle handler on each message has negligible
overhead, and there is no easy way to just attach/remove a single pre-created
idle handler.

+}
+
+void PropertyChangedHandler::DeletePendingSignals() {
+  while (!signal_queue_.empty()) {
+    signal_queue_.pop();
+  }
+}
+
+}  // namespace cashew