[PATCH] client: clarify wl_display_prepare_read() semantics

[Pekka Paalanen]

On Mon,  4 Aug 2014 11:30:46 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:

> From the doc comment I get the feeling, that after successfull call to
> wl_display_prepare_read(), the thread gains exclusive access to the fd.
> That is not true. It only ensures that _one_ of the threads, that called
> this function, will read from the fd and there will be no race.
> 
> Here's slice of the commit message that introduces
> wl_display_prepare_read() (3c7e8bfbb4745315b7bcbf69fa746c3d6718c305):
> 
>   "wl_display_prepare_read() registers the calling thread as a potential
>    reader of events.  Once data is available on the fd, all reader
>    threads must call wl_display_read_events(), at which point
>    one of the threads will read from the fd and distribute the events
>    to event queues.  When that is done, all threads return from
>    wl_display_read_events()."
> 
> That is not clear from the doc message.
> ---
>  src/wayland-client.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 19b5694..d7de24d 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -1155,6 +1155,12 @@ read_events(struct wl_display *display)
>  		pthread_cond_broadcast(&display->reader_cond);
>  	} else {
>  		serial = display->read_serial;
> +		/* in the case that the thread is not allowed to read
> +		 * from the fd, make it sleep until reading and dispatching
> +		 * is done. The condition display->read_serial == serial) is
> +		 * because it may happen, that some thread will call this
> +		 * function after the reading is done and without this, the
> +		 * thread would block until next pthread_cond_broadcast */
>  		while (display->read_serial == serial)
>  			pthread_cond_wait(&display->reader_cond,
>  					  &display->mutex);

I think the condition here is to ensure, that the function call will
not return until someone has actually read the fd, not to avoid
blocking in some cases. Apparently there could be spurious wake-ups,
and need to protect against them. The 'serial' is set to
display->read_serial just before waiting, and the mutex is held, so the
cond_wait is guaranteed to happen at least once.

This branch cannot be entered "after" the reading was done, because
it is always the last call here that reads, when reader_count hits zero.

> @@ -1180,6 +1186,13 @@ read_events(struct wl_display *display)
>   * Before calling this function, wl_display_prepare_read() must be
>   * called first.
>   *
> + * When more threads call wl_display_prepare_read() to announce the
> + * intention to read from the display's fd, all must call this function
> + * (or wl_display_cancel_read()). Only one thread (at random) will succeed
> + * in reading and the others will block in this function
> + * until the reader is done to avoid races. If this function is called
> + * after the reading is done, it returns immediately.

Continuing here, I do not think there is a "after the reading is done".

> + *
>   * \memberof wl_display
>   */
>  WL_EXPORT int
> @@ -1256,15 +1269,21 @@ wl_display_prepare_read_queue(struct wl_display *display,
>   * This function must be called before reading from the file
>   * descriptor using wl_display_read_events().  Calling
>   * wl_display_prepare_read() announces the calling threads intention
> - * to read and ensures that until the thread is ready to read and
> - * calls wl_display_read_events(), no other thread will read from the
> - * file descriptor.  This only succeeds if the event queue is empty
> + * to read from the display's fd. This only succeeds if the event queue is empty
>   * though, and if there are undispatched events in the queue, -1 is
>   * returned and errno set to EAGAIN.
>   *
>   * If a thread successfully calls wl_display_prepare_read(), it must
>   * either call wl_display_read_events() when it's ready or cancel the
> - * read intention by calling wl_display_cancel_read().
> + * read intention by calling wl_display_cancel_read(). After successfull
> + * polling on fd, all threads must call wl_display_read_events() (or cancel).
> + * Only one of them will succeed in reading and once the data are read
> + * and dispatched, all threads return from the function.

The wording is a little strange to me here. When you say only one thread
succeeds, I would think all others fail and get an error. I think it is
also not mandatory to poll, is it? You could just read() and cancel()
right away, no?

> + *
> + * After calling wl_display_prepare_read(), the thread doesn't gain exclusive
> + * access to the fd. It only ensures, that only one thread (at random) will read
> + * from the fd. The non-readers will sleep until the reading is done to avoid races
> + * (see below).
>   *
>   * Use this function before polling on the display fd or to integrate
>   * the fd into a toolkit event loop in a race-free way.  Typically, a

Does it really matter that it might not be exactly this thread that
will actually read the fd? All threads must be coded the same way in any
case.

Are you perhaps thinking of someone doing something else with the fd
than calling wl_display_read_events()? If so, maybe we should just
document the allowed/expected actions?

Or better yet, add a chapter to the publican docs about integrating
libwayland-client to an event loop. I don't recall there being that
yet...


Thanks,
pq