[PATCH 3/4] tests: add display test that highlights another bug in read_events

[Marek Chalupa]

On 10 September 2014 10:53, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Tue, 9 Sep 2014 15:39:35 +0200
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > On 9 September 2014 13:10, Marek Chalupa <mchqwerty at gmail.com> wrote:
> >
> > >
> > >
> > > On 4 September 2014 15:17, Pekka Paalanen <ppaalanen at gmail.com>
> > > wrote:
> > >
> > >> On Fri, 29 Aug 2014 11:21:30 +0200
> > >> Marek Chalupa <mchqwerty at gmail.com> wrote:
> > >>
> > >> > When wl_display_read_events() returns with errno == EAGAIN, we
> > >> > naturally try to call it again. But this next call results in
> > >> > deadlock.
> > >> >
> > >> > Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> > >> > ---
> > >> >  tests/display-test.c | 88
> > >> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >> >  1 file changed, 88 insertions(+)
>
> > >> But I see something else: wl_display_read_events() does not
> > >> decrement reader_count if it returns due to last_error. I think
> > >> that needs a fix to guarantee the reader_count is decremented, so
> > >> that the side-effects are consistent.
> > >>
> > >
> > > Yes, we should fix this. There are cases that can cause a problem.
> > >
> >
> > Actually, I can not make up any use-case where it is problem... do
> > you have any?
>
> No, I just assumed it could be a problem, since it looks like it would
> violate an invariant (keeping the reader count consistent) by
> returning without decrementing. After all, prepare_read already
> succeeded, so it incremented the reader count.
>
>
Yes, we can keep it consistent and decrement it - but after last_error is
set, all read attempts are suppressed, so it should not break anything,
right?
But for the sake of consistency we can do it. After last_error is set, few
instructions are not harmful :)


> I suppose the safest approach would be to maintain the guarantees we
> already had in 1.5.0.
>
>
Hmm, yes. And if program uses poll (as probably most of them uses), then it
should
not reach this problem.


> Should a read_event failing due to last_error wake up all threads? It
> should be completely safe, but OTOH every site setting last_error
> already wakes up all threads and prevents anyone from sleeping in
> read_events again, right?
>
>
Yes, all threads should be woken up by the function that sets last_error,
so this is safe. If we want have it completely consistent, we can
do cancel_read in this case, so that the reader_count is decremented
and if it drops to 0, then it will wake up sleeping threads (although there
should not be any
- but it is not a problem for pthread broadcast)


>
> Thanks,
> pq
>

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