[Cogl] [PATCH 1/2] wayland: Remove the Wayland socket FD if there are any errors

[Robert Bragg]

This looks good to land to me:

Reviewed-by: Robert Bragg <robert at linux.intel.com>

thanks,
- Robert


On Tue, Jul 9, 2013 at 7:26 PM, Neil Roberts <neil at linux.intel.com> wrote:
> Previously if the Wayland socket gets closed then Cogl would ignore
> the error when dispatching events which meant the socket would be
> constantly ready for reading, the main loop would never go idle and it
> would sit at 100% CPU. When Wayland encounters an error it will
> actually close the socket which means if something else opened another
> file then we might even end up polling on a completely unrelated FD.
> This patch makes it remove the FD from the main loop as soon as it
> hits an error so that it will at least avoid breaking the main loop.
> However I think most applications would probably want to abort in this
> case so we might also want to add a way to inform the application of
> this or even just abort directly.
>
> The cogl_poll_* functions have been changed so that they can cope if
> the pending and dispatch callbacks remove their own FD while they are
> invoked.
> ---
>  cogl/cogl-poll.c                      | 24 +++++++++----
>  cogl/winsys/cogl-winsys-egl-wayland.c | 65 +++++++++++++++++++++++++++--------
>  2 files changed, 69 insertions(+), 20 deletions(-)
>
> diff --git a/cogl/cogl-poll.c b/cogl/cogl-poll.c
> index cb62327..f1c19f0 100644
> --- a/cogl/cogl-poll.c
> +++ b/cogl/cogl-poll.c
> @@ -46,23 +46,26 @@ cogl_poll_renderer_get_info (CoglRenderer *renderer,
>                               int *n_poll_fds,
>                               int64_t *timeout)
>  {
> -  GList *l;
> +  GList *l, *next;
>
>    _COGL_RETURN_VAL_IF_FAIL (cogl_is_renderer (renderer), 0);
>    _COGL_RETURN_VAL_IF_FAIL (poll_fds != NULL, 0);
>    _COGL_RETURN_VAL_IF_FAIL (n_poll_fds != NULL, 0);
>    _COGL_RETURN_VAL_IF_FAIL (timeout != NULL, 0);
>
> -  *poll_fds = (void *)renderer->poll_fds->data;
> -  *n_poll_fds = renderer->poll_fds->len;
>    *timeout = -1;
>
>    if (!_cogl_list_empty (&renderer->idle_closures))
>      *timeout = 0;
>
> -  for (l = renderer->poll_sources; l; l = l->next)
> +  /* This loop needs to cope with the prepare callback removing its
> +   * own fd */
> +  for (l = renderer->poll_sources; l; l = next)
>      {
>        CoglPollSource *source = l->data;
> +
> +      next = l->next;
> +
>        if (source->prepare)
>          {
>            int64_t source_timeout = source->prepare (source->user_data);
> @@ -72,6 +75,11 @@ cogl_poll_renderer_get_info (CoglRenderer *renderer,
>          }
>      }
>
> +  /* This is deliberately set after calling the prepare callbacks in
> +   * case one of them removes its fd */
> +  *poll_fds = (void *)renderer->poll_fds->data;
> +  *n_poll_fds = renderer->poll_fds->len;
> +
>    return renderer->poll_fds_age;
>  }
>
> @@ -80,17 +88,21 @@ cogl_poll_renderer_dispatch (CoglRenderer *renderer,
>                               const CoglPollFD *poll_fds,
>                               int n_poll_fds)
>  {
> -  GList *l;
> +  GList *l, *next;
>
>    _COGL_RETURN_IF_FAIL (cogl_is_renderer (renderer));
>
>    _cogl_closure_list_invoke_no_args (&renderer->idle_closures);
>
> -  for (l = renderer->poll_sources; l; l = l->next)
> +  /* This loop needs to cope with the dispatch callback removing its
> +   * own fd */
> +  for (l = renderer->poll_sources; l; l = next)
>      {
>        CoglPollSource *source = l->data;
>        int i;
>
> +      next = l->next;
> +
>        if (source->fd == -1)
>          {
>            source->dispatch (source->user_data, 0);
> diff --git a/cogl/winsys/cogl-winsys-egl-wayland.c b/cogl/winsys/cogl-winsys-egl-wayland.c
> index f5a6047..075c258 100644
> --- a/cogl/winsys/cogl-winsys-egl-wayland.c
> +++ b/cogl/winsys/cogl-winsys-egl-wayland.c
> @@ -132,13 +132,28 @@ prepare_wayland_display_events (void *user_data)
>
>    flush_ret = wl_display_flush (wayland_renderer->wayland_display);
>
> -  /* If the socket buffer became full then we need to wake up the main
> -   * loop once it is writable again */
> -  if (flush_ret == -1 && errno == EAGAIN)
> -    _cogl_poll_renderer_modify_fd (renderer,
> -                                   wayland_renderer->fd,
> -                                   COGL_POLL_FD_EVENT_IN |
> -                                   COGL_POLL_FD_EVENT_OUT);
> +  if (flush_ret == -1)
> +    {
> +      /* If the socket buffer became full then we need to wake up the
> +       * main loop once it is writable again */
> +      if (errno == EAGAIN)
> +        {
> +          _cogl_poll_renderer_modify_fd (renderer,
> +                                         wayland_renderer->fd,
> +                                         COGL_POLL_FD_EVENT_IN |
> +                                         COGL_POLL_FD_EVENT_OUT);
> +        }
> +      else if (errno != EINTR)
> +        {
> +          /* If the flush failed for some other reason then it's
> +           * likely that it's going to consistently fail so we'll stop
> +           * waiting on the file descriptor instead of making the
> +           * application take up 100% CPU. FIXME: it would be nice if
> +           * there was some way to report this to the application so
> +           * that it can quit or recover */
> +          _cogl_poll_renderer_remove_fd (renderer, wayland_renderer->fd);
> +        }
> +    }
>
>    /* Calling this here is a bit dodgy because Cogl usually tries to
>     * say that it won't do any event processing until
> @@ -159,19 +174,41 @@ dispatch_wayland_display_events (void *user_data, int revents)
>    CoglRendererWayland *wayland_renderer = egl_renderer->platform;
>
>    if ((revents & COGL_POLL_FD_EVENT_IN))
> -    wl_display_dispatch (wayland_renderer->wayland_display);
> +    {
> +      if (wl_display_dispatch (wayland_renderer->wayland_display) == -1 &&
> +          errno != EAGAIN &&
> +          errno != EINTR)
> +        goto socket_error;
> +    }
>
>    if ((revents & COGL_POLL_FD_EVENT_OUT))
>      {
>        int ret = wl_display_flush (wayland_renderer->wayland_display);
>
> -      if (ret != -1 || errno != EAGAIN)
> -        /* There is no more data to write so we don't need to wake up
> -         * when the write buffer is emptied anymore */
> -        _cogl_poll_renderer_modify_fd (renderer,
> -                                       wayland_renderer->fd,
> -                                       COGL_POLL_FD_EVENT_IN);
> +      if (ret == -1)
> +        {
> +          if (errno != EAGAIN && errno != EINTR)
> +            goto socket_error;
> +        }
> +      else
> +        {
> +          /* There is no more data to write so we don't need to wake
> +           * up when the write buffer is emptied anymore */
> +          _cogl_poll_renderer_modify_fd (renderer,
> +                                         wayland_renderer->fd,
> +                                         COGL_POLL_FD_EVENT_IN);
> +        }
>      }
> +
> +  return;
> +
> + socket_error:
> +  /* If there was an error on the wayland socket then it's likely that
> +   * it's going to consistently fail so we'll stop waiting on the file
> +   * descriptor instead of making the application take up 100% CPU.
> +   * FIXME: it would be nice if there was some way to report this to
> +   * the application so that it can quit or recover */
> +  _cogl_poll_renderer_remove_fd (renderer, wayland_renderer->fd);
>  }
>
>  static CoglBool
> --
> 1.7.11.3.g3c3efa5
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl