[PATCH] Iteration wakeup

[Fan Wu]

Thanks for reviewing the fix!  I have some comments inline.

> It isn't clear to me why this is not in the same place as the
> wakeup_mainloop (which is currently in
> _dbus_connection_send_preallocated_unlocked_no_update).
>
> I think potentially the wakeup_mainloop should be moved to
> do_iteration_unlocked where you have the wakeup_iteration.
>
> The internal iteration stuff is just a simple main loop.
>
> Maybe the full code is:
>
> else if (connection->n_outgoing > 0 &&
>           (flags & DBUS_ITERATION_DO_WRITING) != 0)
>    {
>       _dbus_connection_wakeup_mainloop (connection);
>       _dbus_transport_wakeup_iteration (connection->transport);
>    }
>
> Note that I have added the check for DO_WRITING, since n_outgoing is
> irrelevant if writing has not been requested.
>
> With this change, the wakeup_mainloop would be removed from
> _dbus_connection_send_preallocated_unlocked_no_update.


The reason I put wakeup_iteration at
_dbus_connection_do_iteration_unlocked() because this function
(_do_iteration_unlocked) is also called by
_dbus_connection_flush_unlocked() and
_dbus_connection_read_write_dispatch(). So put it there shall have
more coverage than put it in
_dbus_connection_send_preallocated_unlocked_no_update().

If you don't mind I'll put your full code into the revised fix and
remove wakeup_mainloop from
_dbus_connection_send_preallocated_unlocked_no_update.


> >  /**by
> > + * This function is used in multi-threaded environment and is used to
> > + * wakeup the DBUS thread if it's in poll/select sleep(). This function
> > + * shall normally be used when an application is about exit.
> > + *
> > + * @param connection the connection.
> > + * @return TRUE if the wakeup mechanism has been executed, otherwise FALSE.
>
> When would you use this return value? (what does it mean exactly?)

The return value of TRUE means the wakeup mechanism has been executed
while most likely FALSE means the DBUS thread is not blocking at the
time the function is called. I don't have a good usage of the return
value at the time. I suggest we leave it there as it is in case we do
come up with something. I'll add more text warning prople about the
multithread context.

> > + */
> > +
> > +dbus_bool_t
> > +dbus_connection_wakeup_iteration (DBusConnection* connection)
>
> I think we'll need more docs and maybe a better name for this function,
> the current public API does not have the idea of "iteration" in it I
> don't think.
>
> Maybe dbus_connection_interrupt_blocking() ? not too good either.

dbus_connection_interrupt_blocking() sounds good to me. I'll take this
one and add more text to the comments.

> > +{
> > +    dbus_bool_t ret = FALSE;
>
> This variable should be named something meaningful, like "executed" or
> whatever it means.
>
> > +
> > +    if (NULL == connection)
>
> dbus code puts this test in the other order, since if you use '='
> accidentally the compiler warns about it anyhow.

That might be a GCC feature. The (latest) MS compiler doesn't complain at all.
>
> > +dbus_bool_t
> > +_dbus_iteration_wakeup (void* data)
> > +{
> > +   DBusIterationWakeupData* p = (DBusIterationWakeupData*)data;
> > +   fd_set wfds;
> > +   struct timeval tv;
> > +   int retval;
> > +
> > +   if (NULL == p || !p->is_polling)
> > +       return FALSE;
> > +
> > +   /* Still do a test before writing*/
> > +   FD_ZERO (&wfds);
> > +   FD_SET (p->wakeup_fd, &wfds);
> > +   tv.tv_sec  = 0;  /*immediate return*/
> > +   tv.tv_usec = 0;
> > +
> > +   retval = select (p->wakeup_fd+1, NULL, &wfds, NULL, &tv);
> > +
> > +   if (retval <= 0)
> > +       return FALSE;
> > +
> > +   /*write one char*/
> > +   if (write (p->wakeup_fd, "w", 1) >0 )
> > +       return TRUE;
>
> This is a race to try to avoid the blocking write this way with
> select(); to avoid the race, you need to use a nonblocking write (i.e.
> set the pipe fd nonblocking - I think there is example code already that
> sets the connection socket nonblocking)

I see. So you are suggesting set the write end of the pipe
nonblocking, and remove the select() in this function, and in this
function just do a write(), correct?

> > +void
> > +_dbus_iteration_wakeup_update (void* data, dbus_bool_t polling_now)
> > +{
> > +   DBusIterationWakeupData* p = (DBusIterationWakeupData*)data;
> > +
> > +   if (NULL == p) return;
> > +
> > +   p->is_polling = polling_now;
> > +}
>
> When is this is_polling flag used?

It's used at the beginning of _dbus_iteration_wakeup():

  > +   if (NULL == p || !p->is_polling)
  > +       return FALSE;

So that if the DBUS thread is not doing polling, the wakeup will  not be run.

>
> We are missing any test coverage for this feature; the minimal tests we
> had involving threads also involved GLib I think so moved with dbus-glib
> to a separate package.
>
> I'm not sure how to suggest writing test coverage here, without adding a
> whole multithread test which I won't ask you to do, but if you can think
> of simple ways to at least call the functions from the tests and be sure
> they don't crash, it would be good.

Do you want the testing to be self-contained (the testing code will
spawn a daemon), or the test code assumes the availability of a
daemon?

Thanks,
Fan