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

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

Avoid libdbus-c++ deadlock issues with sending dbus messages in dbus
signal callbacks by doing as little as possible in these callbacks.

Instead, have the callbacks schedule dbus signal processing to be done
later from the main loop.

This blanket deferral should be simpler and more robust than trying to
defer specific actions such as Service deletion as we've done in
previous CLs.

BUG=chromium-os:8892
TEST=unit tests, manual testing on device (suspend/resume while
connected to 3G)

Change-Id: I05e9026f2ad253d503d6de702b108a4d261237de

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=0f4972d

[Vince Laviano, 2010-11-25 03:28:39]

Here's the first cut at my revamped CL to address the libdbus-c++ deadlock
issue. 

It introduces a PropertyChangedHandler that allows clients to queue up tuples
representing incoming D-Bus PropertyChanged signals and schedules callbacks for
deferred processing of these signals from the glib main loop.

ServiceManager, Service, and Device are all updated to have a
PropertyChangedHandler member component and to implement the associated
PropertyChangedDelegate callback interface. 

Instead of processing incoming PropertyChanged signals immediately, they enqueue
them via their PropertyChangedHandler and are called back later to process them
when not in a libdbus-c++ callback. During this later processing phase, they may
send dbus messages freely in response to the information contained in the
signals.

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

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc
File src/property_changed_handler.cc (right):

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:33: source_id_ =
g_idle_add(StaticOnPropertyChangedCallback, this);
I don't know how well this will work.  Mucking with the mainloop asynchronously
seems like a bad idea.
See below.

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:63: handler->source_id_ = 0;
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").

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:64: return FALSE;
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:07]

Daniel: Thanks for taking a look at the CL. Ping me if you want to discuss these
comments out-of-band.

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc
File src/property_changed_handler.cc (right):

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:33: source_id_ =
g_idle_add(StaticOnPropertyChangedCallback, this);
On 2010/11/29 05:20:17, djkurtz wrote:
> I don't know how well this will work.  Mucking with the mainloop
asynchronously
> seems like a bad idea.
> See below.

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.

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:63: handler->source_id_ = 0;
On 2010/11/29 05:20:17, djkurtz wrote:
> 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.


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.

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

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.

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:64: return FALSE;
On 2010/11/29 05:20:17, djkurtz wrote:
> 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().

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]

LGTM w/ one small potential future optimization to investigate.

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc
File src/property_changed_handler.cc (right):

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:33: source_id_ =
g_idle_add(StaticOnPropertyChangedCallback, this);
OK I agree since it is all single-threaded.

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:63: handler->source_id_ = 0;
OK I agree since it is all single-threaded.

http://codereview.chromium.org/5380002/diff/1/src/property_changed_handler.cc...
src/property_changed_handler.cc:64: return FALSE;
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.