After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 770373 - leak tracer: signal handlers need fixing
leak tracer: signal handlers need fixing
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal blocker
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 767857
Blocks:
 
 
Reported: 2016-08-25 09:35 UTC by Tim-Philipp Müller
Modified: 2018-11-03 12:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tracers/leaks: fix reentrancy issues the the custom signal handlers (3.34 KB, patch)
2016-09-01 07:39 UTC, Matthew Waters (ystreet00)
reviewed Details | Review
tracers: leaks: protect list of leak tracer instances with lock (1.81 KB, patch)
2016-09-10 19:16 UTC, Tim-Philipp Müller
needs-work Details | Review
tracers: leaks: clear mutex and cond var when no longer needed (819 bytes, patch)
2016-09-10 19:17 UTC, Tim-Philipp Müller
needs-work Details | Review
tracers: leaks: clean up signal thread with last tracer instance (6.26 KB, patch)
2016-09-10 19:17 UTC, Tim-Philipp Müller
needs-work Details | Review
tracers/leaks: fix race condition starting up the thread (1.97 KB, patch)
2016-10-21 07:04 UTC, Matthew Waters (ystreet00)
reviewed Details | Review

Description Tim-Philipp Müller 2016-08-25 09:35:55 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.
Comment 1 Matthew Waters (ystreet00) 2016-09-01 07:39:48 UTC
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.
Comment 2 Sebastian Dröge (slomo) 2016-09-01 07:45:50 UTC
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/...
Comment 3 Matthew Waters (ystreet00) 2016-09-01 12:51:03 UTC
(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.
Comment 4 Sebastian Dröge (slomo) 2016-09-01 12:55:06 UTC
Something we can do with gst_deinit() here? That's the case I'm worried about as it will cause noise in tests.
Comment 5 Matthew Waters (ystreet00) 2016-09-02 02:10:58 UTC
(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?).
Comment 6 Sebastian Dröge (slomo) 2016-09-02 06:43:33 UTC
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.
Comment 7 Tim-Philipp Müller 2016-09-10 19:16:03 UTC
Created attachment 335266 [details] [review]
tracers: leaks: protect list of leak tracer instances with lock
Comment 8 Tim-Philipp Müller 2016-09-10 19:17:08 UTC
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)
Comment 9 Tim-Philipp Müller 2016-09-10 19:17:47 UTC
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.
Comment 10 Tim-Philipp Müller 2016-09-10 19:20:35 UTC
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.
Comment 11 Matthew Waters (ystreet00) 2016-09-13 04:14:15 UTC
Looks good to me.
Comment 12 Sebastian Dröge (slomo) 2016-09-13 08:50:49 UTC
Go ahead :)
Comment 13 Tim-Philipp Müller 2016-09-13 09:18:48 UTC
Needs one more small fixup.
Comment 14 Tim-Philipp Müller 2016-09-13 09:53:34 UTC
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.
Comment 15 Sebastian Dröge (slomo) 2016-10-20 09:57:50 UTC
What's the status here?
Comment 16 Matthew Waters (ystreet00) 2016-10-20 12:39:19 UTC
Nothing's changed.
Comment 17 Tim-Philipp Müller 2016-10-20 17:01:50 UTC
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.
Comment 18 Matthew Waters (ystreet00) 2016-10-21 07:04:07 UTC
Created attachment 338153 [details] [review]
tracers/leaks: fix race condition starting up the thread
Comment 19 Sebastian Dröge (slomo) 2016-10-21 11:26:11 UTC
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()?
Comment 20 Matthew Waters (ystreet00) 2016-10-21 12:00:59 UTC
No, this is part 1.

part 2, the fork() thing is not complete yet.
Comment 21 Sebastian Dröge (slomo) 2016-10-24 11:40:55 UTC
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?
Comment 22 Sebastian Dröge (slomo) 2016-10-31 13:07:39 UTC
Postponing and should be listed as a known issue.
Comment 23 Ángel Guzmán Maeso (shakaran) 2017-04-26 10:47:53 UTC
It would be nice get this merged and fixed for at least 1.14 since it is the unique blocker in gstreamer (atm)
Comment 24 Sebastian Dröge (slomo) 2017-04-26 10:48:57 UTC
How does it block you, what exactly are you doing that makes this a big problem for you?
Comment 25 Ángel Guzmán Maeso (shakaran) 2017-04-26 10:55:33 UTC
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.
Comment 26 Sebastian Dröge (slomo) 2017-04-27 09:26:14 UTC
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
Comment 27 GStreamer system administrator 2018-11-03 12:36:00 UTC
-- 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.