[PATCH] client: clarify wl_display_prepare_read() semantics

[Marek Chalupa]

On 28 August 2014 12:02, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> 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.
>
>
Yes, you're right. That's the real purpose of it. My mistake. However,
AFAIK in the current code this loop
will run only once, because everywhere is the cond broadcast called along
with
increasing the serial.


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

It's not mandatory. I took the sentence about polling from the original doc.


> > + *
> > + * 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?
>
>
Well, this piece of doc is not my work, here I took the feeling, that we
must poll before
wl_display_read_events(). However, nowadays I (hopefully) understand how it
works and
will reword it.

Regarding the allowed actions.. do you have something particular in mind?
Theoretically,
programmer can safely use everything that doesn't change
display->readers_count.
(basically everything except wl_display_dispatch_queue).
But I don't know why he would do that - the expected action is just to call
wl_display_read_events
(with possibly poll before that) once you're prepared to read.


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

Thanks for notes,
I'll fix the patch and eventually try to write up some 'big doc' for that.

Cheers,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140828/38703d98/attachment.html>