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