<div dir="ltr"><br><div class="gmail_extra"><br><div>On 4 September 2014 15:17, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, 29 Aug 2014 11:21:30 +0200<br>
Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> When wl_display_read_events() returns with errno == EAGAIN, we<br>
> naturally try to call it again. But this next call results in deadlock.<br>
><br>
> Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>><br>
> ---<br>
> tests/display-test.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++<br>
> 1 file changed, 88 insertions(+)<br>
><br>
> diff --git a/tests/display-test.c b/tests/display-test.c<br>
> index 1289866..e1cf830 100644<br>
> --- a/tests/display-test.c<br>
> +++ b/tests/display-test.c<br>
> @@ -32,6 +32,7 @@<br>
> #include <sys/types.h><br>
> #include <sys/stat.h><br>
> #include <pthread.h><br>
> +#include <poll.h><br>
><br>
> #include "wayland-private.h"<br>
> #include "wayland-server.h"<br>
> @@ -480,6 +481,93 @@ TEST(threading_cancel_read_tst)<br>
> display_destroy(d);<br>
> }<br>
><br>
> +/* set this to 1 when it is right that<br>
> + * the thread is woken up */<br>
> +static int threads_can_be_woken_up = 0;<br>
> +<br>
> +static void *<br>
> +thread_eagain_func(void *data) {<br>
> + struct client *c = data;<br>
> +<br>
> + register_reading(c->wl_display);<br>
> +<br>
> + c->display_stopped = 1;<br>
> + assert(wl_display_read_events(c->wl_display) == 0<br>
> + && errno != EAGAIN);<br>
> + /* if thread was sleeping in read_events, this assert<br>
> + * must pass now */<br>
> + assert(threads_can_be_woken_up && "Thread woke up too early");<br>
> +<br>
> + pthread_exit(NULL);<br>
> +}<br>
> +<br>
> +static void<br>
> +threading_read_eagain(void)<br>
> +{<br>
> + struct client *c = client_connect();<br>
> + pthread_t th1, th2, th3;<br>
> + struct pollfd fds;<br>
> + int i;<br>
> +<br>
> + register_reading(c->wl_display);<br>
> +<br>
> + th1 = create_thread(c, thread_eagain_func);<br>
> + th2 = create_thread(c, thread_eagain_func);<br>
> + th3 = create_thread(c, thread_eagain_func);<br>
> +<br>
> + /* All the threads are sleeping, waiting until read or cancel<br>
> + * is called. Since we have no data on socket waiting,<br>
> + * the wl_connection_read should end up with error and set errno<br>
> + * to EAGAIN => wl_display_read_events() will return 0 and set errno<br>
> + * to EAGAIN. It should not wake up the sleeping threads, so that<br>
> + * this thread can try to read again */<br>
> + alarm(3);<br>
> + assert(wl_display_read_events(c->wl_display) == 0);<br>
> + assert(errno == EAGAIN);<br>
<br>
</div></div>I think there is a problem here. wl_display_read_events() returning 0<br>
means success, and in that case errno may not be set if it was a real<br>
success. There is no EAGAIN to be handled, it's already done inside<br>
read_events().<br>
<br>
In this test it won't be a real success, but you cannot code clients to<br>
check for errno after getting 0 from wl_display_read_events(). <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +<br>
> + /* the same should happen next times - but there was a bug when<br>
> + * the main thread got blocked! */<br>
> + for (i = 0; i < 5; ++i) {<br>
> + assert(wl_display_read_events(c->wl_display) == 0);<br>
<br>
</span>Therefore you are here calling wl_display_read_events() again after it<br>
succeeded! And that IMO is a developer error. <br></blockquote><div><br></div><div>yes<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Besides, I cannot see anywhere where it says that it is ok to call<br>
wl_display_read_events() again even if it failed, without calling<br>
wl_display_prepare_read() first.<br>
<br></blockquote><div><br>Since original piece of code returned 0 on EAGAIN, then I suppose the author<br></div><div>considered this as a success, so yes, wl_display_read_events() is not probably meant<br>to be called again. But consider this:<br>if the thread that got EAGAIN (the only thread that is not sleeping) calls wl_display_prepare_read()<br></div><div>+ wl_display_read_events() again, it works. Problem is that it does not have to call it<br></div><div>right after the failed wl_display_read_events and the other threads are still sleeping.<br></div><div>Moreover, if there are not events on the socket for some time, this one thread is looping in cycle<br>while the other threads are sleeping.<br><br></div><div>What I meant to do was to give the programmer the choice whether to loop or let<br></div><div>all threads try it again (cancel read). But as you said before, 0 is wrong return value for that.<br></div><div>See end of the e-mail for conclusions.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But I see something else: wl_display_read_events() does not decrement<br>
reader_count if it returns due to last_error. I think that needs a fix<br>
to guarantee the reader_count is decremented, so that the side-effects<br>
are consistent.<span class=""><br></span></blockquote><br>Yes, we should fix this. There are cases that can cause a problem.<br> </div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> + assert(errno == EAGAIN);<br>
> + }<br>
> +<br>
> + /* generate something to read and wait until the event comes */<br>
> + wl_proxy_marshal((struct wl_proxy *) c->tc,<br>
> + 0 /* STOP DISPLAY */, 1 /* number */);<br>
> + assert(wl_display_flush(c->wl_display) > 0);<br>
> +<br>
> + fds.fd = wl_display_get_fd(c->wl_display);<br>
> + fds.events = POLLIN;<br>
> + assert(poll(&fds, 1, 3) == 1);<br>
<br>
</span>Is timeout of 3 milliseconds intended?<br></blockquote><div><br></div><div>Nope, my bad.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +<br>
> + /* threads can be woken up now */<br>
> + threads_can_be_woken_up = 1;<br>
> +<br>
> + errno = 0;<br>
> + assert(wl_display_read_events(c->wl_display) == 0);<br>
> + assert(errno != EAGAIN);<br>
> + assert(wl_display_dispatch_pending(c->wl_display) == 1);<br>
> +<br>
> + pthread_join(th1, NULL);<br>
> + pthread_join(th2, NULL);<br>
> + pthread_join(th3, NULL);<br>
> +<br>
> + client_disconnect(c);<br>
> +}<br>
> +<br>
> +TEST(threading_read_eagain_tst)<br>
> +{<br>
> + struct display *d = display_create();<br>
> +<br>
> + client_create(d, threading_read_eagain);<br>
> +<br>
> + display_run(d);<br>
> + display_resume(d);<br>
> +<br>
> + display_destroy(d);<br>
> +}<br>
> +<br>
> static void *<br>
> thread_prepare_and_read2(void *data)<br>
> {<br>
<br>
<br>
</div></div>Thanks,<br>
pq<br>
</blockquote></div><br></div><div class="gmail_extra">First, thanks for review.<br><br></div><div class="gmail_extra">Summing it up, in my point of view we have these options:<br><br></div><div class="gmail_extra">* Let it as it is:<br></div><div class="gmail_extra"> this can lead to starving of other threads for some time. Moreover, if the thread that<br></div><div class="gmail_extra"> got EAGAIN decides to exit (or simply not to read again) instead of another trial to read, the other threads will block.<br></div><div class="gmail_extra">* return -1 and set EAGAIN<br></div><div class="gmail_extra"> programmer can either try to read again, or cancel read and let all threads do another cycle. Good thing is<br> that programmer knows that there was an error and can cancel read in the case that the thread is exiting<br></div><div class="gmail_extra"> instead of another cycle.<br></div><div class="gmail_extra"> * wake up threads on EAGAIN<br></div><div class="gmail_extra"> this solution will force all threads to do another cycle (or whatever they want) and should not have any side effects. It can lead to<br></div><div class="gmail_extra"> big overhead if there are no events on the socket for some time (waking up threads again and again in cycle)<br><br></div><div class="gmail_extra">+ fix the bug with last_error, of course<br><br></div><div class="gmail_extra">Is there something that I'm missing?<br><br></div><div class="gmail_extra">Thanks,<br>Marek<br></div></div>