GNOME Bugzilla – Bug 770373
leak tracer: signal handlers need fixing
Last modified: 2018-11-03 12:36:00 UTC
The signal handlers in the leaks tracer need fixing up. You're not really allowed to do anything meaningful in a signal handler, so it works at all, that's mostly luck. Probably needs to be rewritten using another thread and/or signalfd() or GLib's signalfd wrapper source thingy.
Created attachment 334587 [details] [review] tracers/leaks: fix reentrancy issues the the custom signal handlers This solves the reentrancy issues by running a new thread with a couple of glib's signalfd GSources.
Review of attachment 334587 [details] [review]: Generally looks good to me, more threads always solve problems ;) ::: plugins/tracers/gstleaks.c @@ +630,3 @@ +}; + +/* XXX: this thread is leaked */ You could shut down this thread when the tracer is finalized/unloaded/...
(In reply to Sebastian Dröge (slomo) from comment #2) > Review of attachment 334587 [details] [review] [review]: > > Generally looks good to me, more threads always solve problems ;) > > ::: plugins/tracers/gstleaks.c > @@ +630,3 @@ > +}; > + > +/* XXX: this thread is leaked */ > > You could shut down this thread when the tracer is finalized/unloaded/... That's problematic and why I didn't implement that. The thread is created in class_init not instance_init as I assume the signal handlers should only be registered once (how it was before) no matter how many leaks tracers are instantiated. Adding a class_finalize to a static GType will never be called and GObject warns of that. This leaves possibly using base_init/base_finalize and special casing our single class or registering a dynamic type.
Something we can do with gst_deinit() here? That's the case I'm worried about as it will cause noise in tests.
(In reply to Sebastian Dröge (slomo) from comment #4) > Something we can do with gst_deinit() here? That's the case I'm worried > about as it will cause noise in tests. I don't see anything obvious that can be done. The tracers are in a plugin which doesn't support unloading at all (or do they?).
They don't, no. You could implement this with refcounting around instance_init and finalize of the tracer instances. Keep the thread running as long as there is at least one instance.
Created attachment 335266 [details] [review] tracers: leaks: protect list of leak tracer instances with lock
Created attachment 335267 [details] [review] tracers: leaks: clear mutex and cond var when no longer needed Probably not needed with native linux threads, but possibly on other unices. (Could also be squashed with next patch)
Created attachment 335268 [details] [review] tracers: leaks: clean up signal thread with last tracer instance For static types class_finalize and such won't be called, so start up and stop our signal handling thread in instance init/finalize instead, keeping a count of current users. This way we don't leak the thread and misc stuff with it.
With those, valgrind seems happy enough, apart from: 2,032 bytes in 1 blocks are definitely lost in loss record 1,573 of 1,583 at 0x4C2CBE5: calloc (vg_replace_malloc.c:711) by 0x5A92D00: g_malloc0 (gmem.c:124) by 0x5AAB163: thread_memory_from_self (gslice.c:519) by 0x5AAB163: g_slice_alloc (gslice.c:1003) by 0x5AABF75: g_slist_prepend (gslist.c:254) by 0x5AB50B0: g_once_init_enter (gthread.c:661) by 0x5A8AFAB: g_main_context_new (gmain.c:620) by 0x72A01CB: gst_leaks_tracer_signal_thread (gstleaks.c:665) by 0x5AB4F04: g_thread_proxy (gthread.c:784) by 0x6168463: start_thread (pthread_create.c:333) But this looks like something that just needs to be added to the suppression files to me, it's a GSList node that should be allocated i g_once_enter_init() and freed in g_once_enter_leave(), so I think it's just something kept around by GSlice.
Looks good to me.
Go ahead :)
Needs one more small fixup.
Needs more fixing, just fails left front center and right with 'make check', because we now start the thread, then fork(), then expect to join() it. Apparently it *sometimes* works?! No idea why everything was fine in my previous testing.
What's the status here?
Nothing's changed.
I have changes locally (not sure if patches here are latest version), but I'm still finding it occasionally hangs, so needs further work. We can either revert the SIGUSR support for now or just leave it as-is. I think leaving is ok, it's only enabled on-demand, so no problem by default.
Created attachment 338153 [details] [review] tracers/leaks: fix race condition starting up the thread
Comment on attachment 338153 [details] [review] tracers/leaks: fix race condition starting up the thread This works with running tests "forever" for a long time? I thought the problem is that we sometimes start the thread before fork()?
No, this is part 1. part 2, the fork() thing is not complete yet.
Should we get any of these in now, even if it's only a partial fix so far? Does it improve the situation or is not doing anything and leaking more reliable still? Should we ignore this for 1.10 and document it as a known problem?
Postponing and should be listed as a known issue.
It would be nice get this merged and fixed for at least 1.14 since it is the unique blocker in gstreamer (atm)
How does it block you, what exactly are you doing that makes this a big problem for you?
Hi Sebastian, I am inspecting and learning Pitivi source code. I also developed (some years ago) a small app called Tivion for video streaming (now unmaintained, but reconsidering to relive). From the python side some handlers signals could benefit of this fix and avoid leaks.
This is not about GObject signals but about UNIX signals, and only relevant for the leaks tracer. I don't think any of this can be used from Python. If you observe Python leaks, please file another bug with testcase for that
-- 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/186.