<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On 22 August 2014 11:50, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 22 Aug 2014 09:42:16 +0200 <div><div class="h5">Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote: > On 21 August 2014 15:15, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote: > > > On Wed, 25 Jun 2014 14:35:16 +0200 > > Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote: > > > > > Test if events are going to the right queue and if the queue > > > is interrupted from polling when an error to the main queue comes. > > > The last one is failing. > > > --- > > > tests/queue-test.c | 128 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 128 insertions(+) > > > > > > diff --git a/tests/queue-test.c b/tests/queue-test.c > > > index 36d7a69..307bae9 100644 > > > --- a/tests/queue-test.c > > > +++ b/tests/queue-test.c > > > @@ -27,6 +27,7 @@ > > > #include <sys/types.h> > > > #include <sys/wait.h> > > > #include <assert.h> > > > +#include <pthread.h> > > > > > > #include "wayland-client.h" > > > #include "wayland-server.h" > > > @@ -139,6 +140,53 @@ client_test_multiple_queues(void) > > > exit(ret == -1 ? -1 : 0); > > > } > > > > > > +/* test that events will come to the right queue */ > > > +static void > > > +client_test_multiple_queues2(void) > > > +{ > > > + struct wl_event_queue *queue[5], *q; > > > > Instead of using 5 everwhere, how about a #define? > > > > Yes, that'll be better. > > > > > > > + struct wl_callback *callback; > > > + struct wl_display *display; > > > + int i, j; > > > + > > > + display = wl_display_connect(NULL); > > > + assert(display); > > > + > > > + for (i = 0; i < 5; ++i) { > > > + queue[i] = wl_display_create_queue(display); > > > + assert(queue[i]); > > > + } > > > + > > > + for (i = 0; i < 100; ++i) { > > > + q = queue[i % 5]; > > > + callback = wl_display_sync(display); > > > > Isn't this 'callback' leaked? > > > > > + assert(callback != NULL); > > > + wl_proxy_set_queue((struct wl_proxy *) callback, q); > > > + > > > + assert(wl_display_flush(display) > 0); > > > + > > > + /* the callback should come to the q queue */ > > > + assert(wl_display_dispatch_pending(display) == 0); > > > + assert(wl_display_dispatch_queue(display, q) > 0); > > > + /* now all queues should be empty */ > > > + for (j = 0; j < 5; ++j) > > > + assert(wl_display_dispatch_queue_pending(display, > > > + queue[j]) > > == 0); > > > + } > > > + > > > + callback = wl_display_sync(display); > > > > And this? > > > > I'll add a listener that will destroy the callbacks. > > > > > > > > > > > + assert(callback != NULL); > > > + > > > + assert(wl_display_flush(display) > 0); > > > + assert(wl_display_dispatch(display) > 0); > > > + for (j = 0; j < 5; ++j) { > > > + assert(wl_display_dispatch_queue_pending(display, > > queue[j]) == 0); > > > + wl_event_queue_destroy(queue[j]); > > > + } > > > + > > > + wl_display_disconnect(display); > > > +} > > > + > > > static void > > > dummy_bind(struct wl_client *client, > > > void *data, uint32_t version, uint32_t id) > > > @@ -169,5 +217,85 @@ TEST(queue) > > > client_create(d, client_test_multiple_queues); > > > display_run(d); > > > > > > + client_create(d, client_test_multiple_queues2); > > > + display_run(d); > > > + > > > + display_destroy(d); > > > +} > > > + > > > +struct thread_data { > > > + struct wl_display *display; > > > + struct wl_event_queue *queue; > > > +}; > > > + > > > +static void * > > > +run_new_queue(void *arg) > > > +{ > > > + struct thread_data *data = arg; > > > + int ret; > > > + > > > + /* XXX shouldn't it return -1 in the first dispatch? > > > + * Anyway - the queue keeps polling atm and is not interrupted by > > > + * the error */ > > > > Hmm, yes, I think would probably be good that if wl_display goes into > > error state, all the dispatch calls should return with error, rather > > than hang. > > > > > Actually, this test-case is wrong. When I was writing this, I did not know > anything about threading in Wayland. > Nowadays, I know that this is exactly the case where > wl_display_prepare_read + wl_display_read_events should > be used (more threads are reading from the display's fd), so when > programmer will do what is in this test, > it is his fault. However, I think I could send a path that clarifies this > in the documentation > (I've already sent one, but that is only for wl_display_prepare_read and > there's no note about not using wl_display_dispatch concurrently: > <a href="http://lists.freedesktop.org/archives/wayland-devel/2014-August/016296.html" target="_blank">http://lists.freedesktop.org/archives/wayland-devel/2014-August/016296.html</a>) > > However, this test-case shows that it is possible to have one thread > polling and the other successfully dispatching events > without interrupting the poll, so I'll try to find out if it can happen in > some correct way. </div></div>Oh, ok. So this test as it was written, was also incorrect with the API that was before wl_display_prepare_read? </blockquote><div> </div><div>Hmm, not exactly. AFAIK, before wl_display_prepare_read only the main thread was allowed to read from fd and dispatch events, so there were no races at all. </div><div>So the test would probably pass, since the dispatch in thread would return immediately with an error (wrong thread). </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I don't think that old API was ever removed, nor can be removed, so it should still work according to the old rules. To make things even more complicated, I suppose it should still work, if, say, an app is using the older API and a library is using the latest API. I have no idea if that actually works. </blockquote> The 'old' functions still work, but their are supposed to be used from one thread only (multi-threaded environment is ok, but the functions must be called from the same thread every time - or programmer must somehow serialize their use). Problem can arise when mixing wl_display_prepare_read with wl_display_dispatch(_queue) (even in one thread) or when calling wl_display_dispatch(_queue) from more threads at the same time (without serializing). </div><div class="gmail_quote"> </div><div class="gmail_quote">I think that there are no apps that are calling wl_display_dispatch(_queue) from more threads at the same time, because otherwise we have very likely already got some feedback regarding that xD </div><div class="gmail_quote">The wl_display_prepare_read func that was introduced because wl_display_dispatch is not thread-safe is used in Xwayland IIRC. </div><div class="gmail_quote">I think that the first step here would be properly document it so that people won't use bad functions on bad places. Since no one complained about threading yet, I think that we still have time to </div><div class="gmail_quote">prevent such mistakes. </div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Thanks, pq </blockquote></div> </div><div class="gmail_extra">Thanks, Marek </div></div>