Possible resource leak in Weston

Fri Apr 24 10:50:46 UTC 2020

On Thu, 23 Apr 2020 16:14:43 +0200
Guillermo Rodriguez <guillerodriguez.dev at gmail.com> wrote:

> Hi Pekka,
> 
> El jue., 23 abr. 2020 a las 11:43, Pekka Paalanen
> (<ppaalanen at gmail.com>) escribió:
> >
> > On Wed, 22 Apr 2020 16:45:42 +0200
> > Guillermo Rodriguez <guillerodriguez.dev at gmail.com> wrote:
> >  
> > > Hi all,
> > >
> > > I am investigating something that looks like a resource leak in
> > > Weston. I first saw the problem in an application involving Gstreamer,
> > > which would run out of fds after a number of iterations (~1000).
> > > However I have also been able to reproduce it without using Gstreamer.
> > >
> > > This is the scenario:
> > >
> > > I have a wayland application that uses Gstreamer to display video. The
> > > application creates the display connection (wl_display_connect) and a
> > > top-level surface; these last for the lifetime of the application.
> > > Every time the application needs to show a video fragment, it builds a
> > > Gstreamer pipeline, and passes the display and surface handles to
> > > Gstreamer's waylandsink.
> > >
> > > This ends up calling the gst_wl_display_new_existing function [1],
> > > which registers a listener for the wayland registry and binds to some
> > > interfaces [2]. Binding to the wl_shell interface results in a call to
> > > weston_desktop_wl_shell_bind -> weston_desktop_client_create where
> > > some resources are created, including, among others, a ping timer.
> > >
> > > When Gstreamer is done showing the video, it cleans up and releases
> > > all resources. As part of this process it calls wl_shell_destroy [3],
> > > however this does NOT result in a call to
> > > weston_desktop_client_destroy, and so the associated resources are not
> > > released. In fact, the resources are only released when the
> > > application disconnects (wl_display_disconnect) and exits. At this
> > > point, all "pending" calls to weston_desktop_client_destroy are made.
> > >
> > > The original symptoms (application running out of fds) are only
> > > visible with wayland < 1.18.0. This is because up to 1.18.0, one
> > > timerfd was being used for each ping timer. This was changed in this
> > > commit [4]. However even if the symptoms are less visible, the issue
> > > still exists in Weston.
> > >
> > > Does this make sense?  
> >
> > Hi,
> >
> > yes, it does make sense, I think.
> >  
> > > In case I am not overlooking something and this is indeed a Weston
> > > issue, any hints on how to fix it?  
> >
> > It is mostly a Weston issue, but there are a couple Wayland protocol
> > issues as well:
> >
> > - wl_registry does not have a destroy request defined, hence the
> >   compositor cannot know when a client destroys a wl_registry. The
> >   compositor side of a wl_registry object can only be reaped when the
> >   client disconnects. Fixing this is cumbersome.
> >
> > - wl_shell interface does not have a destroy request either. However,
> >   adding one would be much easier that with wl_registry. OTOH, wl_shell
> >   is deprecated, so even if we add one, I doubt the clients still using
> >   it would get fixed.
> >
> > The above are fundamental but relatively minor leaks during a client's
> > lifetime in the compositor. Solving them should not be necessary for
> > solving your immediate problem, but they would be needed for a complete
> > solution rather than just pushing the failure point even further out.  
> 
> The lack of a destroy request for wl_shell seems to be the root of the
> problem in my case.
> Each time my application needs to show a video fragment it builds a
> Gstreamer pipeline, Gstreamer's waylandsink binds to the wl_shell
> client, and this results in the creation of a new ping timer. Once the
> video stops the timer resources are not released. After a number of
> iterations of this process, the application cannot show video anymore.
> 
> You mention that "solving this should not be necessary for solving my
> immediate problem", but I am not sure why you say that. If the
> wl_shell objects (and associated resources) are being leaked, how do I
> work around this issue?

Hi,

I say that, because there is no need for a compositor to hang any state
on the wl_shell protocol object from the protocol perspective. The only
thing it cannot avoid to leak is one wl_resource per bind to wl_shell.

All state is related to wl_shell_surface objects instead.

IOW, what you are seeing is an implementation "inefficiency" with
implementation object lifetimes.

> > There is also a peculiarity with the wl_shell_surface interface: it
> > does not have a destroy request defined, but the protocol object is
> > documented to be destroyed when the wl_surface is destroyed. You might
> > want to check libweston-desktop actually implements this, and that the
> > client also does destroy the wl_surface.
> >
> > In the early days of Wayland, we missed to add destroy requests to many
> > interfaces which we assumed would only be instantiated once in a
> > client's lifetime. That's why the oldest levels of the protocol
> > interfaces are inconsistent wrt. freeing.
> >
> > The rest is issues with libweston-desktop or weston in general. I'm not
> > too familiar with libweston-desktop, but I have some ideas.
> >
> > To me it looks like libweston-desktop hangs too much data (including
> > the ping timer) on the client. E.g. the ping timer is not needed if
> > there is nothing to ping, so the timer could perhaps be torn down
> > earlier and created on demand. Note however, that while wl_shell pings
> > on the wl_shell_surface, xdg-shell pings on the xdg_wm_base which is
> > not a per-surface object. Fortunately xdg_wm_base does have a destroy
> > request, so hooking up to that is simple.
> >
> > I assume that 'struct weston_desktop_client' is really supposed to be
> > per-client, and not e.g. per wl_shell object.  
> 
> Looks like the struct weston_desktop_client is per shell object. If I
> add a registry listener twice in a row, e.g.:
> 
> ===
> wl_registry_add_listener (WlDisplay.registry, &registry_listener, NULL);
> for (i = 0; i < 2; i++) {
>     if (_wl_display_roundtrip () < 0) {
>         ...handle error
>     }
> }
> 
> wl_registry_add_listener (WlDisplay.registry, &registry_listener, NULL);
> for (i = 0; i < 2; i++) {
>     if (_wl_display_roundtrip () < 0) {
>         ...handle error
>     }
> }
> ===
> 
> then weston_desktop_client_create is called twice....

That's unfortunate. Sounds like libweston-desktop needs some
adjustments in its design.

For instance, it's using one ping timer "per client" which is actually
per bound global. That could be turned into one per client such that
when the first wl_shell_surface gets created, the timer gets created,
and then the last wl_shell_surface gets destroyed, the timer gets
freed. Of course need to factor in the other shell protocols as well.

And maybe weston_desktop_client should hang from the wl_client and be
created on demand, not on bind to wl_shell.

Thanks,
pq

> 
> Thanks,
> 
> Guillermo
> 
> > All wl_shell objects are
> > totally equivalent, so ensuring this would be nice. I'm not sure if all
> > xdg_wm_base objects are intended to be totally equivalent as well,
> > because it contains the ping/pong messages, and I don't how it is
> > supposed to work if a client has multiple xdg_wm_base objects, e.g. can
> > they be used interchangeably to respond to pings.
> >
> >
> > Thanks,
> > pq
> >  
> > >
> > > Thank you,
> > >
> > > Guillermo Rodriguez
> > >
> > >  [1]: https://github.com/GStreamer/gst-plugins-bad/blob/master/ext/wayland/wldisplay.c#L295
> > >  [2]: https://github.com/GStreamer/gst-plugins-bad/blob/master/ext/wayland/wldisplay.c#L204
> > >  [3]: https://github.com/GStreamer/gst-plugins-bad/blob/master/ext/wayland/wldisplay.c#L91
> > >  [4]: https://github.com/wayland-project/wayland/commit/60a8d29ad8526c18cc670612e2c94f443873011a
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel  
> >  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20200424/5e9de760/attachment.sig>