idle tasks starving in toytoolkit

[Pekka Paalanen]

On Thu, 26 Sep 2013 16:56:49 +0100
Neil Roberts <neil at linux.intel.com> wrote:

> One idea to fix this might be to make dispatch_queue only ever
> dispatch the events that were current when the loop is started. That
> way if any further events are added while processing the current
> events it will give control back to the main loop before processing
> them.
> 
> Here's a not-heavily-tested patch that would do that.

Hi Neil,

if you inspect the main loop of toytoolkit, and how main loops in
clients are supposed to be written in general, does the patch below
still guarantee, that all queued events have been dispatched before an
event loop for that queue ends up blocking in epoll?

If yes, could you add an explanation of that too to the commit message,
please?

If not, is there not a possibility to break existing applications by
blocking too early?


Thanks,
pq

> ------- >8 --------------- (use git am --scissors to automatically chop here)
> 
> Subject: [PATCH] client: Don't dispatch events that are added while dispatching
> 
> Previously wl_display_dispatch_queue would keep dispatching events
> until the event queue becomes empty. If more events are queued while
> dispatching those events the loop can run indefinitely. This patch
> changes it so that instead of iterating the list of events directly in
> the queue it steals the list of events before dispatching and iterates
> the local list instead. That way it will only iterate the events that
> were current before the loop started and if further events are added
> the application will drop back to the main loop before processing
> them.
> ---
>  src/wayland-client.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index a20a71e..ae00ef0 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -918,16 +918,12 @@ decrease_closure_args_refcount(struct wl_closure *closure)
>  }
>  
>  static void
> -dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
> +dispatch_event(struct wl_display *display, struct wl_closure *closure)
>  {
> -	struct wl_closure *closure;
>  	struct wl_proxy *proxy;
>  	int opcode;
>  	bool proxy_destroyed;
>  
> -	closure = container_of(queue->event_list.next,
> -			       struct wl_closure, link);
> -	wl_list_remove(&closure->link);
>  	opcode = closure->opcode;
>  
>  	/* Verify that the receiving object is still valid by checking if has
> @@ -1049,19 +1045,36 @@ wl_display_read_events(struct wl_display *display)
>  static int
>  dispatch_queue(struct wl_display *display, struct wl_event_queue *queue)
>  {
> -	int count;
> +	struct wl_closure *closure, *tmp;
> +	struct wl_list event_list;
> +	int count = 0;
>  
>  	if (display->last_error)
>  		goto err;
>  
> -	for (count = 0; !wl_list_empty(&queue->event_list); count++) {
> -		dispatch_event(display, queue);
> +	/* Steal the list of events from the queue so that if more
> +	 * events are added while dispatching the current ones we
> +	 * won't get stuck in this loop indefinitely */
> +	wl_list_init(&event_list);
> +	wl_list_insert_list(&event_list, &queue->event_list);
> +	wl_list_init(&queue->event_list);
> +
> +	wl_list_for_each_safe(closure, tmp, &event_list, link) {
> +		wl_list_remove(&closure->link);
> +		count++;
> +
> +		dispatch_event(display, closure);
> +
>  		if (display->last_error)
> -			goto err;
> +			goto err_queue;
>  	}
>  
>  	return count;
>  
> +err_queue:
> +	/* Put any remaining events back in the queue */
> +	wl_list_insert_list(&queue->event_list, &event_list);
> +
>  err:
>  	errno = display->last_error;
>