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

[Pekka Paalanen]

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(+)
> 
> diff --git a/tests/display-test.c b/tests/display-test.c
> index 1289866..e1cf830 100644
> --- a/tests/display-test.c
> +++ b/tests/display-test.c
> @@ -32,6 +32,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <pthread.h>
> +#include <poll.h>
>  
>  #include "wayland-private.h"
>  #include "wayland-server.h"
> @@ -480,6 +481,93 @@ TEST(threading_cancel_read_tst)
>  	display_destroy(d);
>  }
>  
> +/* set this to 1 when it is right that
> + * the thread is woken up */
> +static int threads_can_be_woken_up = 0;
> +
> +static void *
> +thread_eagain_func(void *data) {
> +	struct client *c = data;
> +
> +	register_reading(c->wl_display);
> +
> +	c->display_stopped = 1;
> +	assert(wl_display_read_events(c->wl_display) == 0
> +		&& errno != EAGAIN);
> +	/* if thread was sleeping in read_events, this assert
> +	 * must pass now */
> +	assert(threads_can_be_woken_up && "Thread woke up too early");
> +
> +	pthread_exit(NULL);
> +}
> +
> +static void
> +threading_read_eagain(void)
> +{
> +	struct client *c = client_connect();
> +	pthread_t th1, th2, th3;
> +	struct pollfd fds;
> +	int i;
> +
> +	register_reading(c->wl_display);
> +
> +	th1 = create_thread(c, thread_eagain_func);
> +	th2 = create_thread(c, thread_eagain_func);
> +	th3 = create_thread(c, thread_eagain_func);
> +
> +	/* All the threads are sleeping, waiting until read or cancel
> +	 * is called. Since we have no data on socket waiting,
> +	 * the wl_connection_read should end up with error and set errno
> +	 * to EAGAIN => wl_display_read_events() will return 0 and set errno
> +	 * to EAGAIN. It should not wake up the sleeping threads, so that
> +	 * this thread can try to read again */
> +	alarm(3);
> +	assert(wl_display_read_events(c->wl_display) == 0);
> +	assert(errno == EAGAIN);

I think there is a problem here. wl_display_read_events() returning 0
means success, and in that case errno may not be set if it was a real
success. There is no EAGAIN to be handled, it's already done inside
read_events().

In this test it won't be a real success, but you cannot code clients to
check for errno after getting 0 from wl_display_read_events().

> +
> +	/* the same should happen next times - but there was a bug when
> +	 * the main thread got blocked! */
> +	for (i = 0; i < 5; ++i) {
> +		assert(wl_display_read_events(c->wl_display) == 0);

Therefore you are here calling wl_display_read_events() again after it
succeeded! And that IMO is a developer error.

Besides, I cannot see anywhere where it says that it is ok to call
wl_display_read_events() again even if it failed, without calling
wl_display_prepare_read() first.

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.

> +		assert(errno == EAGAIN);
> +	}
> +
> +	/* generate something to read and wait until the event comes */
> +	wl_proxy_marshal((struct wl_proxy *) c->tc,
> +			 0 /* STOP DISPLAY */, 1 /* number */);
> +	assert(wl_display_flush(c->wl_display) > 0);
> +
> +	fds.fd = wl_display_get_fd(c->wl_display);
> +	fds.events = POLLIN;
> +	assert(poll(&fds, 1, 3) == 1);

Is timeout of 3 milliseconds intended?

> +
> +	/* threads can be woken up now */
> +	threads_can_be_woken_up = 1;
> +
> +	errno = 0;
> +	assert(wl_display_read_events(c->wl_display) == 0);
> +	assert(errno != EAGAIN);
> +	assert(wl_display_dispatch_pending(c->wl_display) == 1);
> +
> +	pthread_join(th1, NULL);
> +	pthread_join(th2, NULL);
> +	pthread_join(th3, NULL);
> +
> +	client_disconnect(c);
> +}
> +
> +TEST(threading_read_eagain_tst)
> +{
> +	struct display *d = display_create();
> +
> +	client_create(d, threading_read_eagain);
> +
> +	display_run(d);
> +	display_resume(d);
> +
> +	display_destroy(d);
> +}
> +
>  static void *
>  thread_prepare_and_read2(void *data)
>  {


Thanks,
pq