[PATCH 4/4] client: fix bug with display->reader_count < 0

Marek Chalupa mchqwerty at gmail.com
Fri Aug 29 02:21:31 PDT 2014


If we will try call wl_display_read_events() again,
after we got EAGAIN from previous call, we get deadlock
as shown in test. The bug works like this: after first call
to wl_display_read_events() the display->reader_count is 0
and next call will decrease it to -1 so the thread will make
sleeping itself and we have a deadlock.

We have two options here:

Either we can wake up all threads before returning from
read_events() and thus force the programmer to do the whole
prepare+read loop again. Waking up all the threads could bring
unnecessary overhead.

The other solution, that I chose, is to return without waking up
the threads, so that programmer can try read again
- it feels natural after getting EAGAIN. Thus the proper use
of wl_display_read_events should look like:

    do {
        ret = wl_display_read_events(display);
    } while (ret == 0 && errno == EAGAIN);

The solution is simple. Before returning from read_events under
these circumstances, increase the display->reader_count back to 1,
so that next trial will have the same execution path as the one
before.

Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
---
 src/wayland-client.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/wayland-client.c b/src/wayland-client.c
index 4fc7105..6f806f8 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -1154,8 +1154,15 @@ read_events(struct wl_display *display)
 	if (display->reader_count == 0) {
 		total = wl_connection_read(display->connection);
 		if (total == -1) {
-			if (errno == EAGAIN)
+			if (errno == EAGAIN) {
+			/* do not wake up threads, so that the thread that
+			 * is lucky to read can try to read again. We just
+			 * need to increase display->reader_count to 1 again,
+			 * so that in the next trial, the thread will get
+			 * to this branch again */
+				++display->reader_count;
 				return 0;
+			}
 
 			display_fatal_error(display, errno);
 			return -1;
@@ -1179,11 +1186,13 @@ read_events(struct wl_display *display)
 		}
 
 		display_wakeup_threads(display);
-	} else {
+	} else if (display->reader_count > 0) {
 		serial = display->read_serial;
 		while (display->read_serial == serial)
 			pthread_cond_wait(&display->reader_cond,
 					  &display->mutex);
+	} else {
+		assert(0 && "display->reader_count should never be negative");
 	}
 
 	return 0;
-- 
1.9.3



More information about the wayland-devel mailing list