[PATCH] Iteration wakeup

[Havoc Pennington]

Hi,

Sorry for the slow reply, I have not had time to look at this. Thanks 
for the fixes. Here are some comments -

Fan Wu wrote:
> diff -rup dbus-1.1.2/dbus/dbus-connection.c
> dbus-1.1.2-fix/dbus/dbus-connection.c
> --- dbus-1.1.2/dbus/dbus-connection.c	2007-07-24 09:39:08.000000000 -0600
> +++ dbus-1.1.2-fix/dbus/dbus-connection.c	2007-08-03 14:42:02.000000000 -0600
> @@ -1144,6 +1144,8 @@ _dbus_connection_do_iteration_unlocked (
>  				    flags, timeout_milliseconds);
>        _dbus_connection_release_io_path (connection);
>      }
> +   else if ( connection->n_outgoing > 0)
> +      _dbus_transport_wakeup_iteration (connection->transport);
>

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.

>  /**
> + * 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?)

> + */
> +
> +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_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.

> +/*
> + * Functions used to wakeup iteration
> + */
> +dbus_bool_t    _dbus_iteration_wakeup_initialize  (void*       data);
> +dbus_bool_t    _dbus_iteration_wakeup             (void*       data);
> +void           _dbus_iteration_wakeup_free        (void*       data);
> +void           _dbus_iteration_wakeup_reset       (void*       data);
> +void           _dbus_iteration_wakeup_update      (void*       data,
> +                                                   dbus_bool_t polling_now);
> +

These should use an opaque data type, rather than void* - there are 
other examples in the code.

But in essence, in the header file put:
  "typedef struct DBusIterationWakeup DBusIterationWakeup"

then in the .c file, have your DBusIterationWakeupData (but rename it 
DBusIterationWakeup).

Then you don't need void* or casting.

The API should be like:
  DBusIterationWakeup* _dbus_iteration_wakeup_new();
  void                 _dbus_iteration_wakeup_free(DBusIterationWakeup 
*wakeup);

etc.

> +   DBusIterationWakeupData* p =
> (DBusIterationWakeupData*)malloc(sizeof(DBusIterationWakeupData));

Use dbus_new() instead of malloc here.

> +
> +   if (NULL == p) return FALSE;

reverse test and two lines, like:

  if (p == NULL)
     return FALSE;

But I would make this function be
DBusIterationWakeup*
_dbus_iteration_wakeup_new(void)
{
   DBusIterationWakeup *wakeup;

   wakeup = dbus_new(DBusIterationWakeup, 1);
   if (wakeup == NULL)
     return NULL;

   ...
}


> +
> +   if (pipe(fd))
> +   {
> +       free(p);

use dbus_free()

> +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)

> +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?

> +  void*   iteration_wakeup_data;        /**<  Data used by platform
> dependent code
> +                                         *    to interrupt the
> poll/select sleep
> +                                         */

Just make this a DBusIterationWakeup object, which is in the 
cross-platform dbus-sysdeps.h

> +  if (socket_transport->iteration_wakeup_data)
> +         poll_fd[1].fd = *((int*)socket_transport->iteration_wakeup_data);

Rather than doing this, just add a function 
_dbus_iteration_wakeup_get_socket().

> +  else
> +         poll_fd[1].fd = 0;

I would set -1 here, 0 is a valid fd.

> +      if ( poll_fd[1].fd )
> +              poll_fd[1].events |= _DBUS_POLLIN;

if (poll_fd[1].fd >= 0)

> +      poll_res = _dbus_poll (poll_fd, (poll_fd[1].fd?2:1), poll_timeout);

also change to fd >= 0 if fd is set to -1 not 0

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.

Havoc