[PATCH] tests: add multiple-queues testcases
Marek Chalupa
mchqwerty at gmail.com
Fri Aug 22 03:33:30 PDT 2014
On 22 August 2014 11:50, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri, 22 Aug 2014 09:42:16 +0200
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > On 21 August 2014 15:15, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> >
> > > On Wed, 25 Jun 2014 14:35:16 +0200
> > > Marek Chalupa <mchqwerty at gmail.com> 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:
> >
> http://lists.freedesktop.org/archives/wayland-devel/2014-August/016296.html
> )
> >
> > 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.
>
> Oh, ok. So this test as it was written, was also incorrect with the API
> that was before wl_display_prepare_read?
>
>
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.
So the test would probably pass, since the dispatch in thread would return
immediately
with an error (wrong thread).
> 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.
>
>
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).
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
The wl_display_prepare_read func that was introduced because
wl_display_dispatch is not thread-safe
is used in Xwayland IIRC.
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
prevent such mistakes.
>
> Thanks,
> pq
>
Thanks,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140822/fb521438/attachment-0001.html>
More information about the wayland-devel
mailing list