C convenience lib (was Re: Is dbus_pending_call_set_notify() thread safe?)

[Fan Wu]

Hi Havoc,

I am not disagreeing with you that my proposal is not a NEW model. Let
me put it this way, theoretically it's the same as 1)b, but it
simplifies user interface in that user does not need to deal with DBUS
related looping (threads or mainloop). It's not meant to be completely
transparent in hiding the async nature of the model, but hopefully
users can write less code and still get things work. Convenient,
that's all.

The backend can be a thread running a mainloop, or a thread blocking
of dbus_connection_read_write_dispatch(). With the latter approach,
the bug as you mentioned has to be fixed. A workaround is too have two
connections and each with its own thread, but that's just too complex.
I'm working on a patch to fix the bug and will post it here by the end
of this week. What the patch does is to make the poll/select
interruptible, so that whenever a write initiative fails in
acquire_io_path, AND the transport is blocking in the poll call, the
blocking will be interrupted. The patch will be for client only.

Btw, thanks again for the detailed comments. It's very helpful for
understanding the internals of DBUS.
Fan

On 8/2/07, Havoc Pennington <hp at redhat.com> wrote:
> Hi,
>
> Fan Wu wrote:
> > What I'm thinking it's acutally a variant of your 1)b. The only
> > difference is the threading part is hidden in the API.
>
> Here's what I'm confused about. You still have to expose some mechanism
> for async to the app, OR you have to block the app. Meaning, you can't
> really hide the threading part.
>
> The app can see either:
>   - async where threads are visible to the app, i.e. app gets method call
>     reply in a different thread from main thread
>   - async where it assumes some main loop but not a particular one; the
>     app gets a descriptor it has to watch, then when there's stuff on
>     that descriptor the app wakes up the library
>   - not async, the library just blocks on the dbus descriptor
>
> So libdbus already offers all three of those models. It might could have
> a somewhat different API or more convenient API, but no matter what the
> API, the model will be one of the above.
>
> The fourth model is:
>   - async where the library knows the main loop, so rather than giving
>     the app descriptors and timeouts, the library can just add the
>     descriptors and timeouts to the main loop directly
>
> So the fourth model is the only convenient/easy-to-use async model, but
> requires the library to know the main loop, which is why libdbus does
> not offer this model directly.
>
> > So pseudo code
> > would be like this:
> >
> > //this will spawn a worker thread in the background
> > // which will try to connect to the bus,
> > // try to hold a name etc
> > dbus_convenience_api_initialize(bus_name,
> >                                 callbacks,
> >                                 name_to_own...);
> >
> > //in the main thread
> > while(continue_looping) // or poll/select/WaitforMultipleObjects
> > {
> >    ...
> >    if(got_stuff_to_send)
> >    {
> >       //this is the part that needs to wake up worker thread
> >       //in case it's in poll/select sleep
> >       dbus_convenience_api_send(conn, msg);
> >    }
> >    ...
> > }
> >
> > //wake up the worker thread and exit it
> > dbus_convenience_api_finalize(handle)
>
> We could have an API like this but what I'd point out is that the
> callbacks all have to be invoked in the worker thread. That would mean
> that the app "sees" the worker thread; it isn't transparent. The
> callbacks would either have to do all their work in the worker thread
> (which would mean the whole app has to be thread safe), or the callbacks
> would have to "send" work back to the main thread - which would involve
> adding a file descriptor that the main thread would have to watch.
>
> In other words nothing about threading or main loops is substantively
> changed here; all the tradeoffs are still the same as I described in the
> first part of this mail.
>
> I wonder if what you're mainly getting at is just a bug.
>
> Right now, if your DBusConnection is dispatched by a main loop you can
> set a "wakeup main loop" function, which is called e.g. in
> dbus-connection.c:_dbus_connection_send_preallocated_unlocked_no_update()
>
> If you have a main thread that dispatches the connection from a main
> loop, and you dbus_connection_send(), then the wakeup main function will
> "kick" the main loop and the main loop can then write out the message
> you want to send.
>
> However, if you don't have a main loop set up, but instead are using
> dbus_pending_call_block() or send_with_reply_and_block() or
> dbus_connection_read_write_dispatch(), then there is no wakeup mechanism.
>
> So what we could do is, whenever we currently call wakeup_main_function,
> we could *also* wakeup our own poll() if we are currently blocking,
> using an extra pipe as you described.
>
> This seems like a serious bug if you 1) want to send messages from
> multiple threads and 2) are just using dbus blocking calls or
> dbus_connection_read_write_dispatch() rather than setting up a main loop
> on the connection.
>
> Is that the situation you are in?
>
> I'm not sure if this is the bug you are encountering, though?
>
> > The solution I'm thinking about is, add a wakeup handle (unamed
> > pipe/socketpair for Unix, unamed event for windows) to the transport,
> > then in socket_do_iteration() both the data socket and the wakeup
> > handle get polled. If the data socket is blocking, the sleep can be
> > interrupted through the wakeup handle. The next question is the timing
> > of using the wakeup handle. The socket is programmed to block
> > infinitely (unless it's waiting for a reply), so if there is only one
> > sending thread, the sender shall be able to use the wakeup trick
> > whenever it fails to acquire the IO path. If there are multiple sender
> > threads (it's possible but not recommended by the model above), it
> > shall not be a problem by wakeup worker thread multiple times. The
> > worst case is the worker wakes up and find outgoing queue is empty. In
> > this case it shall go back to blocking again.
> >
> > This wakeup feature is intended for client only.
> >
> > Thoughts?
>
> This seems like a reasonable bug fix, however I think it's just an
> internal bug fix not necessarily new API - basically we need a
> wakeup_main_function for the poll() dbus itself does when you block
> instead of using the application's main loop.
>
> New API seems like it would be a separate/orthogonal kind of discussion,
> right?
>
> Havoc
>