[PATCH 2/2] client: wake up sleeping threads on all return paths from read_events

[Marek Chalupa]

Hmm, thinking about that... Maybe this is not the right solution.
Programmer should probably do something like:

do {
  ret = wl_display_read_events(display);
} while (ret == 0 && errno == EAGAIN);

to be sure he read the events (or got an error). This patch would force him
to re-run wl_display_prepare_read again
in all threads.


On 28 August 2014 11:32, Marek Chalupa <mchqwerty at gmail.com> wrote:

> In the first place, create a function display_wakeup_threads that
> contains always-repeated pattern: broadcast + increase serial.
>
> Replace the pattern with this function and call this function also
> on return from read_events when wl_connection_read fails with
> EAGAIN (this solves the bug from the test from previous commit).
>
> Now all return paths from read_events are safe and wake up the threads.
> There's a path in wl_display_read_events that do not wake up
> threads - this is the case when last_error is set. But on this
> path are the threads always awake, because every function that can
> set last_error sets it via display_fatal_error (which calls
> display_wakeup_threads).
>
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
>  src/wayland-client.c | 42 +++++++++++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 9159ee0..9574cf7 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -110,6 +110,27 @@ struct wl_display {
>  static int debug_client = 0;
>
>  /**
> + * This helper function wakes up all threads that are
> + * waiting for display->reader_cond (i. e. when reading is done
> + * canceled, or an error occured)
> + *
> + * NOTE: must be called with display->mutex locked
> + */
> +static void
> +display_wakeup_threads(struct wl_display *display)
> +{
> +       pthread_cond_broadcast(&display->reader_cond);
> +
> +       /* Thread can get sleeping only in read_events() and if we're
> +       * waking it up, it means that the read processed or was
> +       * canceled, so we must increase the read_serial.
> +       * This prevents from indefinite looping in read_events()
> +       * (when calling pthread_cond_wait under condition
> +       * display->read_serial == serial). */
> +       ++display->read_serial;
> +}
> +
> +/**
>   * This function is called for local errors (no memory, server hung up)
>   *
>   * \param display
> @@ -128,11 +149,7 @@ display_fatal_error(struct wl_display *display, int
> error)
>
>         display->last_error = error;
>
> -       pthread_cond_broadcast(&display->reader_cond);
> -       /* prevent from indefinite looping in read_events()
> -       * (when calling pthread_cond_wait under condition
> -       * display->read_serial == serial) */
> -       ++display->read_serial;
> +       display_wakeup_threads(display);
>  }
>
>  /**
> @@ -182,7 +199,7 @@ display_protocol_error(struct wl_display *display,
> uint32_t code,
>         display->protocol_error.interface = intf;
>
>         /*
> -        * here it is not necessary to broadcast reader's cond like in
> +        * here it is not necessary to wake up threads like in
>          * display_fatal_error, because this function is called from
>          * an event handler and that means that read_events() is done
>          * and woke up all threads. Since wl_display_prepare_read()
> @@ -1138,8 +1155,10 @@ read_events(struct wl_display *display)
>         if (display->reader_count == 0) {
>                 total = wl_connection_read(display->connection);
>                 if (total == -1) {
> -                       if (errno == EAGAIN)
> +                       if (errno == EAGAIN) {
> +                               display_wakeup_threads(display);
>                                 return 0;
> +                       }
>
>                         display_fatal_error(display, errno);
>                         return -1;
> @@ -1162,8 +1181,7 @@ read_events(struct wl_display *display)
>                         }
>                 }
>
> -               display->read_serial++;
> -               pthread_cond_broadcast(&display->reader_cond);
> +               display_wakeup_threads(display);
>         } else {
>                 serial = display->read_serial;
>                 while (display->read_serial == serial)
> @@ -1348,10 +1366,8 @@ wl_display_cancel_read(struct wl_display *display)
>         pthread_mutex_lock(&display->mutex);
>
>         display->reader_count--;
> -       if (display->reader_count == 0) {
> -               display->read_serial++;
> -               pthread_cond_broadcast(&display->reader_cond);
> -       }
> +       if (display->reader_count == 0)
> +               display_wakeup_threads(display);
>
>         pthread_mutex_unlock(&display->mutex);
>  }
> --
> 1.9.3
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140828/3750f323/attachment-0001.html>