[PATCH] client: clarify wl_display_prepare_read() semantics

[Marek Chalupa]

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

> 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.
>
>
Oh, didn't know that. Anyway, I wouldn't even think of removing the serial
- it's working xD
and it is an assurance for future extensions.


>
> > 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. :-)
>
>
When I first ran into that, the word 'subtly' was not in place xD


> Thanks,
> pq
>

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