GNOME Bugzilla – Bug 755003
Incomplete invalidation involving sigc::slot(functor) and sigc::compose
Last modified: 2015-11-06 09:46:57 UTC
Created attachment 311294 [details] demo program This is observed under MSVC 9.0SP1 and gcc 4.8 with libsigc++ 2.2.9-2.4.0. Compare sigc::slot<void> c = sigc::compose(sigc::mem_fun(&obj, &Obj::Set), sigc::mem_fun(&obj2, &Obj::Get)); signal.connect(c); to sigc::slot<void, value_t> setter = sigc::mem_fun(&obj, &Obj::Set); sigc::slot<void> c = sigc::compose(setter, sigc::mem_fun(&obj2, &Obj::Get)); signal.connect(c); In the second case, invalidation of the setter functor doesn't invalidate the enclosing composed functor. This results in various resource leaks which are especially noticeable when the setter is short-lived compared to its getter sibling and the signal the composed slot connects to. 1. The composed functor still retains a reference to the getter slot (sigc::compose's 2nd param) (and all bound params), and invoking the composed functor still invokes the getter slot, even though the result is thrown away. 2. If the composed functor is connected to a signal, it is never removed from the signal's list of connected slots. Interestingly, if one passes the functor *directly* to sigc::compose (i.e. w/o first wrapping with sigc::slot), setter invalidation completely invalidates the composed functor. Please refer to the attached demo program for a concrete example. Compile with -DWANT_LEAK_SLOT to use slot indirection and observe the leak.
Could you try this with a more recent libsigc++, please? And could you try to create a test case in the style of the existing libsigc++ test cases, maybe even as a patch? https://git.gnome.org/browse/libsigc++2/tree/tests Or at least, with some output to show when the error is happening or not.
I tested it yesterday with the latest version from the git repository. valgrind reported memory leaks, much more with WANT_LEAK_SLOT defined than without it. It would be difficult to create a test case that reports a memory leak in itself. The easiest way to test it is with valgrind or a similar program.
(In reply to Murray Cumming from comment #1) > Could you try this with a more recent libsigc++, please? Sure. AFAICT it's reproducible with libsigc++ 2.5.4. > And could you try to create a test case in the style of the existing > libsigc++ test cases, maybe even as a patch? > https://git.gnome.org/browse/libsigc++2/tree/tests Okay. Will attach shortly.
Created attachment 311584 [details] [review] Diff against test_disconnect.cc demonstrating bug 755003
Created attachment 311616 [details] [review] Test case with track_obj I was wrong. It was not difficult to create a good test case. If sig.connect(sigc::compose(setter, sigc::ptr_fun(&foo))); is replaced by sig.connect(sigc::compose(sigc::track_obj(setter, a3), sigc::ptr_fun(&foo))); the slot is disconnected as expected. A slot within an adaptor is perhaps problematic. I wonder if it's because sigc::slot is not derived from sigc::trackable. Might be interesting to test with other adaptors than sigc::compose.
Another workaround: { A a3; auto setter = sigc::mem_fun(&a3, &A::foo); sig.connect(sigc::compose(setter, sigc::ptr_fun(&foo))); } (A a3 instead of A a, because A a does not compile with -Wshadow.) The problem with a sigc::slot in an adaptor is not that sigc::slot is not derived from sigc::trackable. The functor returned by sigc::mem_fun() is not derived from sigc::trackable. The important difference is that there are specializations of struct sigc::visitor<> for all functors that sigc::mem_fun() can create, but none for sigc::slot. slot.h contains the comment "Because slot is opaque, visit_each() will not visit its internal members."
Created attachment 314820 [details] [review] patch: slot: Handle auto-disconnection when a slot contains a slot Here's a patch that seems to fix the bug. With this patch, there is no extra memory leak in the demo program in comment 0 (only the deliberate leak). libsigc++'s test suite passes, including the new tests in test_disconnect.cc. I suspect that no one has really thought of a case like this: A slot that contains another slot. The normal way of finding all referenced objects that derive from sigc::trackable, does not work, when the outer slot is created. When visit_each() comes to a slot, it stops there. It can't figure out which objects the inner slot refers to. My solution is to let the outer slot become the parent of the inner slot. Then the outer slot is informed, when an object that the inner slot refers to, is deleted. It's a bit convoluted, but the alternatives I've thought of are worse. Ryan, you are welcome to do more tests before I push the patch.
> Ryan, you are welcome to do more tests before I push the patch. That's awesome. I began unpacking sigc++ and figuring out slot internals just last week in order to look into this, but you've beaten me to the punch. AFAICT, the patch looks good. Thanks, Kjell!
Review of attachment 314820 [details] [review]: I have pushed the patch. https://git.gnome.org/browse/libsigc++2/commit/?id=1ad7f93386aed6afab67249413c8ebef05882b67