[PATCH wayland v2 1/2] client: update obsolete comments

[Pekka Paalanen]

On Fri, 28 Nov 2014 12:18:55 +0100
Marek Chalupa <mchqwerty at gmail.com> wrote:

> 1) there is nothing like main thread since
>    3c7e8bfbb4745315b7bcbf69fa746c3d6718c305 anymore, so remove
>    it from documentation and update the doc accordingly.
> 
> 2) use calling 'default queue' instead of 'main queue'. In the code
>    we use display->default_queue, so it'll be easier the understand.
> 
> 3) update some obsolete or unprecise pieces of documentation
> 
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
>  src/wayland-client.c | 80 ++++++++++++++++++++++++++++++++--------------------
>  src/wayland-client.h | 26 ++++++++---------
>  2 files changed, 61 insertions(+), 45 deletions(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 36380fe..0ab94e9 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -1333,18 +1333,19 @@ wl_display_prepare_read_queue(struct wl_display *display,
>   * wl_display_dispatch(display);
>   * \endcode
>   *
> - * There are two races here: first, before blocking in poll(), the fd
> - * could become readable and another thread reads the events.  Some of
> - * these events may be for the main queue and the other thread will
> - * queue them there and then the main thread will go to sleep in
> - * poll().  This will stall the application, which could be waiting
> - * for a event to kick of the next animation frame, for example.
> - *
> - * The other race is immediately after poll(), where another thread
> - * could preempt and read events before the main thread calls
> - * wl_display_dispatch().  This call now blocks and starves the other
> + * The race is immediately after poll(), where one thread
> + * could preempt and read events before the other thread calls
> + * wl_display_dispatch(). This call now blocks and starves the other
>   * fds in the event loop.
>   *
> + * Another race would be when using more event queues.
> + * When one thread calls wl_display_dispatch(_queue)(), then it
> + * reads all events from display's fd and queues them in appropriate
> + * queues. Then it dispatches only its own queue and the other events
> + * are sitting in their queues, waiting for dispatching. If that happens
> + * before the other thread managed to call poll(), it will
> + * block with events queued.
> + *
>   * A correct sequence would be:
>   * \code
>   * while (wl_display_prepare_read(display) != 0)
> @@ -1358,9 +1359,12 @@ wl_display_prepare_read_queue(struct wl_display *display,
>   * Here we call wl_display_prepare_read(), which ensures that between
>   * returning from that call and eventually calling
>   * wl_display_read_events(), no other thread will read from the fd and
> - * queue events in our queue.  If the call to
> - * wl_display_prepare_read() fails, we dispatch the pending events and
> - * try again until we're successful.
> + * queue events in our queue. If the call to wl_display_prepare_read() fails,
> + * we dispatch the pending events and try again until we're successful.

> + * If we have more queues, we may want to use
> + * wl_display_dispatch_queue_pending() instead of wl_display_dispatch_pending(),
> + * to be sure that we dispatched all the queues (not only the default one).
> + * Also use \ref wl_display_prepare_read_queue() in this case.

That is inaccurate. wl_display_dispatch_queue_pending() will dispatch
only one queue, but the text seems to imply it might dispatch all, and
that the default queue would need to be dispatched always.

The wl_display_prepare_read_queue() needs to always match the
wl_display_dispatch_queue_pending().

Each thread should dispatch only the queue(s) it owns, never any other
queues, and I feel also that is not clearly communicated in this.

If a thread has multiple queues, there is no requirement to handle them
all at the same time. Each queue usually has a different context where
it needs to dispatch, and dispatching elsewhere would be a problem.

How about the following:

"If the relevant queue is not the default queue, then
wl_display_prepare_read_queue() and wl_display_dispatch_queue_pending()
need to be used instead."

>   *
>   * \memberof wl_display
>   */
> @@ -1399,10 +1403,25 @@ wl_display_cancel_read(struct wl_display *display)
>   * Dispatch all incoming events for objects assigned to the given
>   * event queue. On failure -1 is returned and errno set appropriately.
>   *
> - * This function blocks if there are no events to dispatch. If calling from
> - * the main thread, it will block reading data from the display fd. For other
> - * threads this will block until the main thread queues events on the queue
> - * passed as argument.
> + * The behaviour of this function is exactly the same as the behaviour of
> + * wl_display_dispatch(), but it dispatches events on given queue,
> + * not on the default queue.
> + *
> + * This function blocks if there are no events to dispatch. When this
> + * function returns after blocking, we can be sure that if some events
> + * for given queue came, they are dispatched now. Since we can not be sure
> + * that among incoming events are those for our queue, we need to check
> + * the return value. If it is equal to 0, it means that
> + * this function read events from fd and queued them to appropriate queues,
> + * but since there were no events for our queue, it dispatched none.

> + * Make sure you'll dispatch pending events on the other queues too in
> + * this case.

This last sentence: again, each queue needs to be dispatched from the
correct context (thread, locks held, etc.), so I suggest rephrasing it
more like: "Make sure also the other queues get dispatched in their
appropriate context (thread, locks held, etc.)." Or maybe just leave it
out, since saying no other queues got dispatched should be enough.

> +
> + * \note Display queue (for wl_display events like delete_id or so)
> + * is dispatched always.

I wonder if referring to the display queue is just confusing, because I
don't see us really explaining what it is anywhere. And it's not the
default queue. In fact, I don't think the users of libwayland would even
care about it.

> + *
> + * \sa wl_display_dispatch(), wl_display_dispatch_pending(),
> + * wl_display_dispatch_queue_pending()
>   *
>   * \memberof wl_display
>   */
> @@ -1499,17 +1518,20 @@ wl_display_dispatch_queue_pending(struct wl_display *display,
>   * \param display The display context object
>   * \return The number of dispatched events on success or -1 on failure
>   *
> - * Dispatch the display's main event queue.
> + * Dispatch the display's default event queue.
>   *
> - * If the main event queue is empty, this function blocks until there are
> + * If the default event queue is empty, this function blocks until there are
>   * events to be read from the display fd. Events are read and queued on
> - * the appropriate event queues. Finally, events on the main event queue
> + * the appropriate event queues. Finally, events on the default event queue
>   * are dispatched.
>   *
> - * \note It is not possible to check if there are events on the main queue
> - * or not. For dispatching main queue events without blocking, see \ref
> + * \note It is not possible to check if there are events on the queue
> + * or not. For dispatching default queue events without blocking, see \ref
>   * wl_display_dispatch_pending().
>   *
> + * \note This function always dispatches display queue (the queue
> + * for wl_display events - this is different from default queue)

The same about the display queue here. I think it's more of an
implementation detail than anything a user would care about.

> + *
>   * \sa wl_display_dispatch_pending(), wl_display_dispatch_queue()
>   *
>   * \memberof wl_display
> @@ -1520,7 +1542,7 @@ wl_display_dispatch(struct wl_display *display)
>  	return wl_display_dispatch_queue(display, &display->default_queue);
>  }
>  
> -/** Dispatch main queue events without reading from the display fd
> +/** Dispatch default queue events without reading from the display fd
>   *
>   * \param display The display context object
>   * \return The number of dispatched events or -1 on failure
> @@ -1532,8 +1554,8 @@ wl_display_dispatch(struct wl_display *display)
>   * This is necessary when a client's main loop wakes up on some fd other
>   * than the display fd (network socket, timer fd, etc) and calls \ref
>   * wl_display_dispatch_queue() from that callback. This may queue up
> - * events in the main queue while reading all data from the display fd.
> - * When the main thread returns to the main loop to block, the display fd
> + * events in other queues while reading all data from the display fd.
> + * When the main loop returns from the handler, the display fd
>   * no longer has data, causing a call to \em poll(2) (or similar
>   * functions) to block indefinitely, even though there are events ready
>   * to dispatch.
> @@ -1542,16 +1564,14 @@ wl_display_dispatch(struct wl_display *display)
>   * client should always call wl_display_dispatch_pending() and then
>   * \ref wl_display_flush() prior to going back to sleep. At that point,
>   * the fd typically doesn't have data so attempting I/O could block, but
> - * events queued up on the main queue should be dispatched.
> + * events queued up on the default queue should be dispatched.
>   *
>   * A real-world example is a main loop that wakes up on a timerfd (or a
>   * sound card fd becoming writable, for example in a video player), which
>   * then triggers GL rendering and eventually eglSwapBuffers().
>   * eglSwapBuffers() may call wl_display_dispatch_queue() if it didn't
>   * receive the frame event for the previous frame, and as such queue
> - * events in the main queue.
> - *
> - * \note Calling this makes the current thread the main one.
> + * events in the default queue.
>   *
>   * \sa wl_display_dispatch(), wl_display_dispatch_queue(),
>   * wl_display_flush()
> @@ -1735,7 +1755,7 @@ wl_proxy_get_class(struct wl_proxy *proxy)
>   * \param queue The event queue that will handle this proxy
>   *
>   * Assign proxy to event queue. Events coming from \c proxy will be
> - * queued in \c queue instead of the display's main queue.
> + * queued in \c queue instead of the display's default queue.

Correct up until "instead", because I believe we nowadays have queue
inheritance: a new proxy will inherit the queue of the proxy it was
created with. Some adjustment to that might be nice here (and verifying
that I'm not lying.)

>   *
>   * \sa wl_display_dispatch_queue()
>   *
> diff --git a/src/wayland-client.h b/src/wayland-client.h
> index dd42d7b..71cd822 100644
> --- a/src/wayland-client.h
> +++ b/src/wayland-client.h
> @@ -71,7 +71,7 @@ struct wl_proxy;
>   * added to a queue. On the dispatch step, the handler for the incoming
>   * event set by the client on the corresponding \ref wl_proxy is called.
>   *
> - * A wl_display has at least one event queue, called the <em>main
> + * A wl_display has at least one event queue, called the <em>default
>   * queue</em>. Clients can create additional event queues with \ref
>   * wl_display_create_queue() and assign \ref wl_proxy's to it. Events
>   * occurring in a particular proxy are always queued in its assigned queue.
> @@ -80,31 +80,27 @@ struct wl_proxy;
>   * called by assigning that proxy to an event queue and making sure that
>   * this queue is only dispatched when the assumption holds.

(Btw. the above paragraph explains the term "context", even if it
doesn't call it that.)

>   *
> - * The main queue is dispatched by calling \ref wl_display_dispatch().
> - * This will dispatch any events queued on the main queue and attempt
> - * to read from the display fd if its empty. Events read are then queued
> - * on the appropriate queues according to the proxy assignment. Calling
> - * that function makes the calling thread the <em>main thread</em>.
> + * The default queue is dispatched by calling \ref wl_display_dispatch().
> + * This will dispatch any events queued on the default queue and attempt
> + * to read from the display fd if it's empty. Events read are then queued
> + * on the appropriate queues according to the proxy assignment.
>   *
>   * A user created queue is dispatched with \ref wl_display_dispatch_queue().
> - * If there are no events to dispatch this function will block. If this
> - * is called by the main thread, this will attempt to read data from the
> - * display fd and queue any events on the appropriate queues. If calling
> - * from any other thread, the function will block until the main thread
> - * queues an event on the queue being dispatched.
> + * This function behaves exactly the same as wl_display_dispatch()
> + * but it dispatches given queue instead of the default queue.
>   *
>   * A real world example of event queue usage is Mesa's implementation of
>   * eglSwapBuffers() for the Wayland platform. This function might need
> - * to block until a frame callback is received, but dispatching the main
> + * to block until a frame callback is received, but dispatching the default
>   * queue could cause an event handler on the client to start drawing
>   * again. This problem is solved using another event queue, so that only
>   * the events handled by the EGL code are dispatched during the block.
>   *
> - * This creates a problem where the main thread dispatches a non-main
> + * This creates a problem where a thread dispatches a non-default
>   * queue, reading all the data from the display fd. If the application
>   * would call \em poll(2) after that it would block, even though there
> - * might be events queued on the main queue. Those events should be
> - * dispatched with \ref wl_display_dispatch_pending() before
> + * might be events queued on the default queue. Those events should be
> + * dispatched with \ref wl_display_dispatch_(queue_)pending() before
>   * flushing and blocking.
>   */
>  struct wl_display;

Everything I didn't comment about looks good. I really appreciate going
through all this.

One more thing we perhaps should add: the current threading
support/model was first released in Wayland 1.2, right? We might want
to add a warning somewhere, that Wayland before 1.2 behaved differently
wrt. threads. (another patch, maybe)

Another note is that the private display queue was added pretty late in
b9eebce0aa5559855d835e403ba3bb5960baaadc which makes it first released
in Wayland 1.5. That's probably not important to mention, though.


Thanks,
pq