[PATCH] client: Add acquire-fd API to avoid requiring a polling main thread

[Uli Schlachter]

On 25.03.2013 18:42, Kristian Høgsberg wrote:
[...]
> @@ -847,45 +973,105 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
>  }
>  
>  static int
> -dispatch_queue(struct wl_display *display,
> -	       struct wl_event_queue *queue, int block)
> +read_events(struct wl_display *display, struct wl_event_queue *queue)
>  {
> -	int len, size, count, ret;
> -
> -	pthread_mutex_lock(&display->mutex);
> -
> -	if (display->last_error)
> -		goto err_unlock;
> +	struct pollfd pfd[2];
> +	int len, size, ret;
> +	char c;
> +
> +	if (display->reader_state == NO_READER) {
> +		/* A thread gets here when it's the first reader to
> +		 * try to read and there's LOCKED_READER.  We set
> +		 * VOLUNTEER_READER and poll on the fd and the
> +		 * reader_pipe, so that a locked reader can preempt if
> +		 * it needs to. */
> +		display->reader = pthread_self();
> +		display->reader_state = VOLUNTEER_READER;
> +
> +		pthread_mutex_unlock(&display->mutex);
> +
> +		pfd[0].fd = display->fd;
> +		pfd[0].events = POLLIN;
> +		pfd[1].fd = display->reader_pipe[0];
> +		pfd[1].events = POLLIN;
> +		ret = poll(pfd, 2, -1);
> +		if (pfd[1].revents & POLLIN)
> +			read(display->reader_pipe[0], &c, 1);
> +
> +		pthread_mutex_lock(&display->mutex);
> +
> +		pthread_cond_signal(&display->pipe_cond);
> +
> +		if (ret == -1) {
> +			display_fatal_error(display, errno);
> +			return -1;
> +		}
>  
> -	ret = wl_connection_flush(display->connection);
> -	if (ret < 0 && errno != EAGAIN) {
> -		display_fatal_error(display, errno);
> -		goto err_unlock;
> +		/* If we come back from poll in NO_READER state,
> +		 * another thread locked and then released the fd.
> +		 * Nobody is going to read anything in this state
> +		 * so return instead of blocking without a reader. */
> +		if (display->reader_state == NO_READER)
> +			return 0;

This can no longer happen, either the reader_state is VOLUNTEER_READER or
LOCKED_READER at this point (right?).

>  	}
>  
> -	if (block && wl_list_empty(&queue->event_list) &&
> -	    pthread_equal(display->display_thread, pthread_self())) {
> +	if (pthread_equal(display->reader, pthread_self())) {
> +		/* At this point we have a reader.  Either a
> +		 * VOLUNTEER_READER and the fd is readable or a
> +		 * LOCKED_READER which is allowed to block in
> +		 * wl_connection_read(). */
>  		len = wl_connection_read(display->connection);
>  		if (len == -1) {
>  			display_fatal_error(display, errno);
> -			goto err_unlock;
> +			return -1;
>  		} else if (len == 0) {
>  			display_fatal_error(display, EPIPE);
> -			goto err_unlock;
> +			return -1;
>  		}
>  		while (len >= 8) {
>  			size = queue_event(display, len);
>  			if (size == -1) {
>  				display_fatal_error(display, errno);
> -				goto err_unlock;
> +				return -1;
>  			} else if (size == 0) {
>  				break;
>  			}
>  			len -= size;
>  		}
> -	} else if (block && wl_list_empty(&queue->event_list)) {
> -		pthread_cond_wait(&queue->cond, &display->mutex);
> -		if (display->last_error)
> +
> +		/* Indicate we're done by setting NO_READER and wake
> +		 * up other threads waiting to read. */
> +		if (display->reader_state != LEGACY_READER)
> +			display->reader_state = NO_READER;
> +		pthread_cond_broadcast(&display->reader_cond);
> +	} else {
> +		/* Some other thread signed up to read from the fd, so
> +		 * just wait for the reader to do the work. */
> +		pthread_cond_wait(&display->reader_cond, &display->mutex);
> +	}

Ok, I think I found another problem:

- Thread 1 is a volunteer reader, sitting in poll()
- The server sends some data and poll() returns because display->fd is readable
- Thread 2 calls wl_display_acquire_fd() and reaches the cond_wait(pipe_cond)
- Thread 1 reaches the above cond_wait(reader_cond)
- Thread 2 normally reads from the connection and wakes up all readers.

At this point, everything is OK. No thread has any problems. However, there is
still a single byte sitting in the pipe that was not read. So what happens next?

- Thread X wants to read from the display and becomes a volunteer reader. Since
the pipe is readable, it immediately continues, reads the byte and ends up in
the above cond_wait(reader_cond). At this point we have a thread waiting on
reader_cond, but no reader which will wake it up (=exactly the situation that
inspired this patch, just a lot more unlikely).

Of course, all of this is highly unlikely to happen, but it might happen
nevertheless. :-)

My suggestion would be to move the read() into wl_display_acquire_fd() and have
the VOLUNTEER_READER only exit from poll() without reading from the pipe.

So wl_display_acquire_fd() would do:

    if (old_state == VOLUNTEER_READER) {
        write(display->reader_pipe[1], &c, 1);
        pthread_cond_wait(&display->pipe_cond, &display->mutex);
        read(display->reader_pipe[0], &c, 1);
    }

> +	return 0;
> +}
> +
> +static int
> +dispatch_queue(struct wl_display *display,
> +	       struct wl_event_queue *queue, int block)
> +{
> +	int ret, count;
> +
> +	pthread_mutex_lock(&display->mutex);
> +
> +	if (display->last_error)
> +		goto err_unlock;
> +
> +	ret = wl_connection_flush(display->connection);
> +	if (ret < 0 && errno != EAGAIN) {
> +		display_fatal_error(display, errno);
> +		goto err_unlock;
> +	}
> +
> +	if (block && wl_list_empty(&queue->event_list)) {
> +		if (read_events(display, queue) == -1)
>  			goto err_unlock;
>  	}
>  
> @@ -965,7 +1151,8 @@ wl_display_dispatch_queue_pending(struct wl_display *display,
>   * or not. For dispatching main queue events without blocking, see \ref
>   * wl_display_dispatch_pending().
>   *
> - * \note Calling this makes the current thread the main one.
> + * \note Calling this will release the display file descriptor if this
> + * thread acquired it using wl_display_acquire_fd().
>   *
>   * \sa wl_display_dispatch_pending(), wl_display_dispatch_queue()
>   *
> @@ -974,7 +1161,8 @@ wl_display_dispatch_queue_pending(struct wl_display *display,
>  WL_EXPORT int
>  wl_display_dispatch(struct wl_display *display)
>  {
> -	display->display_thread = pthread_self();
> +	if (display->reader_state == LEGACY_READER)
> +		display->reader = pthread_self();

Doesn't this comparison need to be protected? This could race with another
thread calling wl_display_lock_fd() and changing reader_state away from
LEGACY_READER?

However, I guess this is no issue, because only the "main thread" is allowed to
lock the fd and dispatch the display, right?

>  	return dispatch_queue(display, &display->queue, 1);
>  }
> @@ -1020,7 +1208,8 @@ wl_display_dispatch(struct wl_display *display)
>  WL_EXPORT int
>  wl_display_dispatch_pending(struct wl_display *display)
>  {
> -	display->display_thread = pthread_self();
> +	if (display->reader_state == LEGACY_READER)
> +		display->reader = pthread_self();

Same as above.

>  	return dispatch_queue(display, &display->queue, 0);
>  }
> diff --git a/src/wayland-client.h b/src/wayland-client.h
> index 578fa7e..2efd01c 100644
> --- a/src/wayland-client.h
> +++ b/src/wayland-client.h
> @@ -152,6 +152,9 @@ int wl_display_flush(struct wl_display *display);
>  int wl_display_roundtrip(struct wl_display *display);
>  struct wl_event_queue *wl_display_create_queue(struct wl_display *display);
>  
> +int wl_display_acquire_fd(struct wl_display *display);
> +void wl_display_release_fd(struct wl_display *display);
> +
>  void wl_log_set_handler_client(wl_log_func_t handler);
>  
>  #ifdef  __cplusplus


-- 
- Buck, when, exactly, did you lose your mind?
- Three months ago. I woke up one morning married to a pineapple.
  An ugly pineapple... But I loved her.