Backtrace:

+ Trace 235989

I guess something there is leaking fds and then at some point you can't create new fds. Either the bus is leaked here, or only the GSource of the watch.

gst_bus_remove_watch() is not called automatically when disposing. Is this the application's job or should it be automatically removed when the bus is destroyed?

Is the bus actually destroyed then? Doesn't the GSource contain a reference to the bus?

Indeed the main bus isn't ever destroyed.

At the moment the application needs to remove the watch and it will run forever (left it running for 5 minutes here while previously it would only run for about 30s).

We could remove the watch from the pipeline dispose/finalize but it would be asymmetric as it would be added from the application code and disappear somewhere inside GStreamer. I'd prefer to make the application responsible for it. We could provide helper functions to add or remove watches for the pipeline bus.

Or we could make the GSource have a weak reference to the GstBus, and destroy it together with the bus.

Created attachment 322244 [details] [review]
bus: change GstBusSource to hold a weak ref to GstBus

Patch to use weak refs. My concern here is if this could be racy if a dispatch
is being called at the same time the last bus reference is unrefd.

If you still have the mainloop running while you unref a GstPipeline, couldn't
this happen?

Use GWeakRef and let it give you a strong reference to the GstBus whenever needed... and if that gives you NULL you just don't do anything. However GWeakRef is also not threadsafe currently IIRC (but it should!).

Actually GWeakRef claims to be threadsafe now. Good :)

Additionally I just found out that gst-play does this:


  play->bus_watch = gst_bus_add_watch (GST_ELEMENT_BUS (play->playbin),
      play_bus_msg, play);

and later:
  gst_object_unref (play->playbin);
  g_source_remove (play->bus_watch);


As the g_source_remove is done after the pipeline unref, the bus_watch id will be invalid.

I think you shouldn't remove the GSource but only let the GSource become useless and let the GstBus go away. It's the users responsibility to get rid of GSources that are not needed anymore.

Thiago, can you update the patch? That way we don't change any behaviour, continue to leak the GSource (well, the application is!) but don't leak our GstBus and its fds. The GSource is useless anyway if it's the only thing that still has a reference to the GstBus, nothing can ever post messages on it anymore.

Created attachment 322367 [details] [review]
bus: change GstBusSource to hold a weak ref to GstBus

Updated to not destroy the GSource.

I think this will be racy if the mainloop is still running, the bus
can be freed while the _dispatch() is called and it will lead to
crashes.

IMHO if we are enforcing the application to unref its GSource then
it is better to let the GSource keep a regular ref. Having a leak
is better than causing crashes.

Comment on attachment 322367 [details] [review]
bus: change GstBusSource to hold a weak ref to GstBus

Use GWeakRef and the race conditions goes away

Created attachment 322392 [details] [review]
bus: change GstBusSource to hold a weak ref to GstBus

Had not seen the new GWeakRef stuff, patch updated.

Comment on attachment 322392 [details] [review]
bus: change GstBusSource to hold a weak ref to GstBus

Great, I like this :) In the GST_WARNING() please also put that it's the application's job to destroy the GSource. Maybe even make this a g_warning()

commit 894c67e642c0f858b5b18097fa7c995bf3cc50c1
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Wed Feb 24 10:56:24 2016 -0300

    bus: change GstBusSource to hold a weak ref to GstBus
    
    When holding a regular ref it will cause the GstBus to never
    reach 0 references and it won't be destroyed unless the application
    explicitly calls gst_bus_remove_signal_watch().
    
    Switching to weakref will allow the GstBus to be destroyed.
    The application is still responsible for destroying the
    GSource.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=762552

This and the commit from https://bugzilla.gnome.org/show_bug.cgi?id=762849#c5 were reverted now as they caused problems. Specifically the gst-rtsp-server unit tests were failing.

Let's revisit this for 1.9.

Anybody up for looking into this?

1.11 then perhaps! :)

The problem is that we do 'g_source_remove_poll' on a GSource that was destroyed by rtsp-media. Can't think on a thread-safe way to fix this.

rtsp creates a bus watch and then it has children added to it. When it destroy + unref it isn't the last reference because of the child. Later it will unref the bus and (as the gsource dispose wasn't called) the bus will try to remove the poll, but the gsource was destroyed so this fails.

The g_source_destroy has already removed the polls. We could check if the gsource was destroyed before removing polls but this isn't thread-safe. No idea how to properly fix this.

So the GSource with the polls is destroyed by RTSP, but references to it are still around. And when the source is actually unreffed for the last time, we try to remove the polls (which were already removed during destroy?). And there's no way of getting notified about destroy AFAICS.

Could we let the removal of the polls just always be handled by destroy and never do that ourselves?

(In reply to Sebastian Dröge (slomo) from comment #23)
> So the GSource with the polls is destroyed by RTSP, but references to it are
> still around. And when the source is actually unreffed for the last time, we
> try to remove the polls (which were already removed during destroy?). And
> there's no way of getting notified about destroy AFAICS.

It actually happens when the GstBus is unref'd and we do g_source_remove_poll on the destroyed gsource.

> 
> Could we let the removal of the polls just always be handled by destroy and
> never do that ourselves?

Can we call destroy() from the gstbus dispose? What if the GSource is still in use by something else external?

(In reply to Thiago Sousa Santos from comment #24)

> Can we call destroy() from the gstbus dispose? What if the GSource is still
> in use by something else external?

That seems ok to do, yes.

If something else is still using it, it will just stop getting any "events" from it and it can't be added to a new main context anymore at that point. I don't see any problems here.

I actually made a mistake debugging this. It is the g_source_attach that keeps a reference on the gsource, but the rest holds true.

Using g_source_destroy fixes the rtsp tests but breaks pipelines/gio in -base. It tries to do a g_source_remove after the bus has been destroyed. Not sure this is a valid usecase. Should users expect the GSource to outlive the GstBus?

"gstcheck.c:79:F:general:test_memory_stream:0: Unexpected critical/warning: Source ID 1 was not found when attempting to remove it"

I start to wonder if we are not creating more holes by fixing this and should just tell users to properly clean up the watches after they are not needed anymore.

Usually GSources that are created and returned to the caller (g_idle_source_new(), etc) are to be managed there. That is, the caller is responsible for attaching/removing them to a main context, the caller might have additional references to it around, etc.

Maybe there is just some API missing in GLib to allow doing something sensible here?

*** Bug 785714 has been marked as a duplicate of this bug. ***

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/159.