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

[Kristian Høgsberg]

On Mon, Mar 25, 2013 at 01:24:53PM -0700, Thiago Macieira wrote:
> On segunda-feira, 25 de março de 2013 13.42.31, Kristian Høgsberg wrote:
> > which returns the display fd and blocks any thread trying to read from
> > the fd.  Calling wl_display_dispatch() will read events, release the fd
> > and the dispatch events.  Alternatively,  if after acquiring and polling
> > the fd, a thread decides to not call wl_display_dispatch() after all,
> > the fd can be released by calling wl_display_release_fd().  This is
> > typically useful when poll returns without data on the fd, eg in case of
> > timeout.
> 
> Wouldn't it be better to require calling display_release in all cases? That 
> is, avoid the magic of releasing the file descriptor inside 
> wl_display_dispatch?

It would be less magic, but wl_display_dispatch() reads events and
then dispatches them.  The dispatch part could take a long time and we
need to release the fd so other threads can read their own events
during that time.

> This would add support for dispatching more than once while in a consistent 
> state.

wl_display_dispatch() reads and dispatches all events for the main
event queue.  If you want to poll the fd again, you have to acquire
the fd again.

> >  /** \endcond */
> > @@ -518,6 +529,10 @@ wl_display_connect_to_fd(int fd)
> >  	wl_event_queue_init(&display->queue, display);
> >  	wl_list_init(&display->event_queue_list);
> >  	pthread_mutex_init(&display->mutex, NULL);
> > +	pthread_cond_init(&display->reader_cond, NULL);
> > +	pthread_cond_init(&display->pipe_cond, NULL);
> > +	display->reader_state = LEGACY_READER;
> > +	display->reader = pthread_self();
> > 
> >  	wl_map_insert_new(&display->objects, WL_MAP_CLIENT_SIDE, NULL);
> > 
> > @@ -540,9 +555,118 @@ wl_display_connect_to_fd(int fd)
> >  		return NULL;
> >  	}
> > 
> > +	if (pipe2(display->reader_pipe, O_CLOEXEC) == -1) {
> > +		wl_connection_destroy(display->connection);
> > +		wl_map_release(&display->objects);
> > +		close(display->fd);
> > +		free(display);
> 
> Need to pthread_cond_destroy the two condition variables here.

Yes, and the mutex... and in the error case above this one too.

> > + * by calling wl_display_dispatch().  Simplified, we have:
> > + *
> > + *   wl_display_dispatch_pending(display);
> > + *   poll(fds, nfds, -1);
> > + *   wl_display_dispatch(display);
> > + *
> 
> > + * 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
> > + * fds in the event loop.
> 
> This one could be solved by calling wl_dispatch_pending in all
> cases, couldn't it?

wl_dispatch_pending() doesn't do I/O (doesn't read from the socket),
it only dispatched events queued up in the main event queue.  When
coming out of poll we always want to read - there are no pending
events and there's data in the socket.

> 
> > +WL_EXPORT int
> > +wl_display_acquire_fd(struct wl_display *display)
> > +{
> > +	char c = 0;
> > +	int old_state;
> > +
> > +	pthread_mutex_lock(&display->mutex);
> > +
> > +	if (display->reader_state == LOCKED_READER &&
> > +	    !pthread_equal(display->reader, pthread_self())) {
> > +		errno = EBUSY;
> > +		pthread_mutex_unlock(&display->mutex);
> > +		return -1;
> 
> Do we need the pthread_equal check? Just let it return EBUSY if
> someone tries to acquire twice in the same thread. You're not
> counting the number of locks, so this could lead to an unexpected
> releasing if someone is nesting acquires.

Most mainloops have a prepare-to-block type of callback, which is
where we're going to call wl_display_acquire_fd():

  prepare_to_block(void *data)
  {
    struct wl_display *display = data;

    wl_display_acquire_fd(display);
    wl_display_dispatch_pending(display);
  }

which ensures we got to sleep in the mainloop without pending events
and nobody else trying to read from the fd.  The problem is that
everytime something wakes up the mainloop (timeout or data on other
fds, for example), this callback is called when we go back to sleep.
In other words, we may get called multiple times without the
corresponding wl_display_dispatch() (or wl_display_release_fd())
getting called.

It's not a counted lock, is a "lock if not already locked" behavior.
And there will only be the one dispatch/release when we come out of
poll, not one per wakeup-before-getting-data.

> > +WL_EXPORT void
> > +wl_display_release_fd(struct wl_display *display)
> > +{
> > +	pthread_mutex_lock(&display->mutex);
> > +
> > +	if (display->reader_state != LOCKED_READER ||
> > +	    !pthread_equal(display->reader, pthread_self())) {
> > +		pthread_mutex_unlock(&display->mutex);
> > +		return;
> > +	}
> 
> Ditto. If someone releases without acquiring (or releases from the wrong 
> thread), their code is wrong.

Yeah, this is a programmer error.  Do you have a suggestion how to
better handle it?  I don't want to assert, not sure there's a point in
returning an error, maybe logging a warning is the best option?

> > @@ -597,6 +721,8 @@ wl_display_disconnect(struct wl_display *display)
> >  	pthread_mutex_destroy(&display->mutex);
> >  	if (display->fd > 0)
> >  		close(display->fd);
> > +	close(display->reader_pipe[0]);
> > +	close(display->reader_pipe[1]);
> 
> Missing pthread_cond_destroy here too.

Yeah, added.

> > @@ -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
> 
> "and there's [no] LOCKED_READER", I guess?

Yes, thanks.

Kristian