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

[Marek Chalupa]

This patch is now obsolete, see
http://lists.freedesktop.org/archives/wayland-devel/2014-August/016966.html


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

> 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/20140829/9373d9ef/attachment.html>