[PATCH] client: clarify wl_display_prepare_read() semantics

[Pekka Paalanen]

On Thu, 28 Aug 2014 15:59:37 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:

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

The manual I could find:
http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_wait.html
says that spurious wakeups may occur. So better keep the serial
there.


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

Nothing particular in my mind, I was just wondering if there was
any old API that might break this. OTOH, that can be written off as
just not using the new API correctly.

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

Very good. I think this is one of the areas that are easy to get
subtly wrong, so a good doc is valuable. :-)


Thanks,
pq