[Dbus-cxx-devel] Race hack fix

[Patrick A. <pa...@ph...>]

Hi all:

The watch/dispatch race fix in dbus-cxx 0.6 still needs a bit more
tweaking, unfortunately. The current fix does: after a watch is handled,
it sets need_initiate_processing_hack, and then if that's set, it wakes
up the dispatch thread. 

That's not exactly the problem, though - the dispatch thread will just
wake up, see that it has nothing to do (since the dispatch status is
complete) and go back to sleep. In fact, to remind, the problem is:

1) watch thread wakes up, select() says there's data to read
2) calls handle_read() on that Watch
3) This queues a message, which changes the dispatch status, which wakes
the dispatch thread.
4) The dispatch thread dispatches the message, which calls
Connection::send for the reply. The watch thread still holds the IO
path, so this message gets silently queued
5) The watch thread finishes handle_read().

In the current hack, all that happens next is that
6) the watch thread wakes up the dispatch thread again
7) the dispatch thread sees the dispatch status is DISPATCH_COMPLETE and
wonders why it was woken up before going back to sleep.

There are two ways to actually fix this (well, 3 really):
1) Set a mutex between the watch thread and the dispatch thread such
that you never dispatch while handling watches, and you never handle
watches while dispatching. For some reason I couldn't get this to work.
It should be simple: just a mutex lock around each if (FD_ISSET()) {}
block, and a mutex lock around the (*ci)->dispatch() calls. I probably
just screwed it up simply somehow.

2a) At the end of the watch thread, loop over the connections, and if
there are messages pending, call Connection::flush on them. This is
sadly a bit brute force because there could be messages pending due to
the socket being full, in which case the thread will block
unnecessarily. It's also a pain because the watch thread doesn't loop
over connections at all, so it's not that elegant.

2b) Change DBus::Connection::signal_add_watch to take a Connection
object as well as the watch object, and add a DBus::Connection pointer
inside the Watch. Then, after the watch thread completes, loop over all
watches in m_write_watches, and then if
watch->connection()->has_messages_to_send && !watch->is_enabled() , call
watch->connection()->flush(). You could do this with no changes at all
currently, if you matched the watch->unix_fd() to the
connection->unix_fd() but that seems complex. Not sure it's as simple as
this, though, because the question of ownership of the DBus::Connection
object exists, so maybe matching via the unix_fd() etc. is needed (geh).

Patrick