GNOME Bugzilla – Bug 614765
pulsesink: racy stream status leave implementation
Last modified: 2011-07-05 14:41:25 UTC
commit dbb8a331 added stream status support to pulsesink. I probably needs to ref pulsesink before pa_mainloop_api_once() and releasing the ref() there. GLib-GObject-CRITICAL **: g_object_ref: assertion `object->ref_count > 0' failed aborting... Program received signal SIGABRT, Aborted.
+ Trace 221223
Thread 3034962800 (LWP 26577)
Created attachment 157837 [details] [review] pulsesink: fix racy shutdown Keep a ref of pulsesink for deferred mainloop invocation. Fixes #614765
Comment on attachment 157837 [details] [review] pulsesink: fix racy shutdown Looks good, please push :)
commit 7cf9967e0b65fcf264487344dadfdacf33778ad3 Author: Stefan Kost <ensonic@users.sf.net> Date: Sat Apr 3 23:39:20 2010 +0300 pulsesink: fix racy shutdown Keep a ref of pulsesink for deferred mainloop invocation. Fixes #614765
This is still crashing with 0.10.22 - see https://bugzilla.redhat.com/show_bug.cgi?id=589455 for details and a stack trace. The error is: Assertion '!in_worker(m)' failed at pulse/thread-mainloop.c:161, function pa_threaded_mainloop_stop(). Aborting. To reproduce: while gst-launch-0.10 filesrc location=/usr/share/mail-notification/new-mail.wav ! decodebin ! audioconvert ! pulsesink; do true; done and wait a bit.
commit 5332287e2d8f9dc7b32da0f6618a0730bd98513a Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Thu May 6 13:51:59 2010 +0200 pulsesink: Create and free the PA mainloop in NULL->READY/READY->NULL This fixes a race condition, when stopping the mainloop during finalization is done from a mainloop callback. Fixes bugs #614765 and #590662.
*** Bug 620341 has been marked as a duplicate of this bug. ***
*** Bug 623591 has been marked as a duplicate of this bug. ***
Didn't happen to me since the update so it seems indeed to be fixed.
*** Bug 625442 has been marked as a duplicate of this bug. ***
I saw the same "Assertion '!in_worker(m)' failed " that Benjamin found - bug 625442 It's rare though. if the pulse callback function is the LAST one being called, it will finalize the pulsesink and calling pa_threaded_mainloop_stop() from the pathread, which is prohibited, because if you take a look of pa_threaded_mainloop_stop(), it calls pthread_join(), you CANNOT pthread_join() within the same thread.
Re-opening: The shutdown phase is still racy. Passing the reference to the callback is an incomplete solution, as it is still possible that the mainloop is stopped and cleaned up before the callback got a chance to run. In this case, the reference is leaked. This showed up in Nokia N9 as a shortage of file descriptors, because libpulse keeps the control sockets around. The correct solution is to wait for the callback(s) to be fired (libpulse has no DestroyNotify mechanism for the userdata argument). As comment #10 points out, the reference passing can be eliminated altogether since an actual cleanup from the callback would just deadlock/assert anyways. Attaching proposed patches...
Created attachment 191210 [details] [review] First cleanup patch
Created attachment 191211 [details] [review] Second cleanup patch
Created attachment 191212 [details] [review] Proposed fix
Looks fine. Just not clear about the commenting w.r.t. to the possibility that a cleanup could occur from a callback, which seems currently not possible since the pa_mainloop is stopped going to NULL ---and so no more callbacks--- before sink can be finalized.
Yeah, I thought that would explain the assertion failure from that other bug. Either way, the reference passing is worthless since the callbacks are being waited for again.
I think it can really happen if you have multiple sinks. The mainloop only gets stopped if the last sink is cleaning up.
Indeed. Thanks. commit ae87731de5dc9a1f39edd2b4dd12db30ad1ff0fd Author: René Stadler <rene.stadler@nokia.com> Date: Wed Jun 29 20:59:26 2011 +0300 pulsesink: prevent race condition causing ref leak Since commit 8bfd80, gst_pulseringbuffer_stop doesn't wait for the deferred call to be run before returning. This causes a race when READY->NULL is executed shortly after, which stops the mainloop. This leaks the element reference which is passed as userdata for the callback (introduced in commit 7cf996, bug #614765). The correct fix is to wait in READY->NULL for all outstanding calls to be fired (since libpulse doesn't provide a DestroyNotify for the userdata). We get rid of the reference passing from 7cf996 altogether, since finalization from the callback would anyways lead to a deadlock. Re-fixes bug #614765. commit f8456e2a1aaa9ae3a146ffb2a4510bff581eb88d Author: René Stadler <rene.stadler@nokia.com> Date: Mon Jul 4 08:58:14 2011 +0300 pulsesink: small cleanup of copy-paste code commit 3589cee762e8bb6bf4da6bb52a58554b61de66a0 Author: René Stadler <rene.stadler@nokia.com> Date: Wed Jun 29 19:50:42 2011 +0300 pulsesink: remove unused member variable and misleading log message Wim changed it in commit 8bfd80 so that pa_defer_ran is not read anywhere. The log message used to annotate a mainloop_wait call which is gone.