[PATCH] tests: add multiple-queues testcases

[Pekka Paalanen]

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?

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.


Thanks,
pq