GNOME Bugzilla – Bug 694253
occasional /gdbus/unref-pending test failure
Last modified: 2013-03-28 16:09:21 UTC
I'm seeing an occasional failure in /gdbus/unref-pending test. Happens maybe 1 time in 100. (/home/desrt/code/glib/gio/tests/.libs/lt-gdbus-close-pending:27884): GLib-GObject-CRITICAL **: g_closure_ref: assertion `closure->ref_count > 0' failed Maybe this is related to bug 692034
(gdb) thread apply all bt
+ Trace 231537
Thread 1 (Thread 0x7ffff7fcd7c0 (LWP 23399))
Backtrace seems to implicate bug 118536 instead.
Pretty sure this is actually due to insufficient refcounting when dropping the signal lock in the 'fastpath' va-args case in gsignal...
Created attachment 237046 [details] [review] signals: No need to use atomics for Handler refcount handler_ref and handler_unref_R are always called with the signal lock held. This is obvious for handler_unref_R as it even sometimes drops this lock, and can be verified quickly for handler_ref by looking at all call sites. This improves the performace about 1% on the emit-handled and the emit-handled-generic tests.
Created attachment 237047 [details] [review] signals: Ensure we ref handler in emission fast path We need to keep a reference to the handler in the fast path, just like in the slow path, otherwise if another thread disconnects the handler we may destroy the closure while we're using it without the lock held. We also move the freeing of the instance to after the emission is totally done as the handler_unref_R (and the tracepoint) reference it.
Ugh, i miscalculated the performance increase, its about 6%. The performance losses with the second patch is < 1% at worst (due to the first patch making ref/unref cheap).
Review of attachment 237046 [details] [review]: This looks fine. Might be worth taking a peek at http://git.gnome.org/browse/glib/commit/?id=39ea11ce6b107bf3969a2f94807675b458a5a887 for other possible cases to improve.
Review of attachment 237047 [details] [review]: ::: gobject/gsignal.c @@ +3275,3 @@ TRACE(GOBJECT_SIGNAL_EMIT_END(signal_id, detail, instance, instance_type)); + if (closure != NULL) Why is this needed? In the case that the closure is from a handler, the handler ref will keep the closure alive. If emission hook, we won't be on the fast path. If a class closure handler (or C vtable pointer) then it can't possibly be removed...
Review of attachment 237047 [details] [review]: ::: gobject/gsignal.c @@ +3275,3 @@ TRACE(GOBJECT_SIGNAL_EMIT_END(signal_id, detail, instance, instance_type)); + if (closure != NULL) It unrefs the instance...
Also, i've been running the original /gdbus/unref-pending test for a looong time now and i don't get the critical anymore.
Review of attachment 237047 [details] [review]: ::: gobject/gsignal.c @@ +3275,3 @@ TRACE(GOBJECT_SIGNAL_EMIT_END(signal_id, detail, instance, instance_type)); + if (closure != NULL) lol. Sorry. But why does that need to be all the way out here? ie: what are you trying to fix by moving it?
Review of attachment 237047 [details] [review]: ::: gobject/gsignal.c @@ +3275,3 @@ TRACE(GOBJECT_SIGNAL_EMIT_END(signal_id, detail, instance, instance_type)); + if (closure != NULL) The handler_unref_R call passes the instance, so we need to be below that. Additionally the tracepoing passes the instance too, so better also be after that.
Review of attachment 237047 [details] [review]: Ok. You got me. I was just looking for a way to avoid looking lazy by ACKing both of your patches without comment.
pushed both.
*** Bug 682066 has been marked as a duplicate of this bug. ***