GNOME Bugzilla – Bug 303896
signal's slot list is empty during signal emition - patch needs documentation
Last modified: 2005-08-01 06:58:35 UTC
Please describe the problem: Due to resolution of bug 166217 during signal (with accumulator) emition signal's slot list is empty with all its slots containing in templ_slot_list. Then list is moved to begin of slot list. All above leads to: 1) impossibility to add slot to any position except end during emition (using signal.slots().push_front()) (annoying me) 2) impossibility to recurcively emit same signal (as consequence) Steps to reproduce: 1. create signal with accumulator, which for example stops emition as soon as slot returns false 2. create and connect slot that adds another slot to this signal using push_front on slot_list and connect is to signal, connecting slot return true, connected slot return false 3. emit signal 4. emit signal again Actual results: Slot intended to be first invoked second Expected results: Slot connected to front invoked first and stop emition (due to accumulator) Does this happen every time? Other information: It seems here can be some workarounds: - delay connection (what I currently do), but this looks like huge hack in my program - use iterators, returned by connect (didn't try), but it seems they become invalid during signal emition as well Most intuitive resoltion seems to copy slot_list to temp_slot_list but not move
Created attachment 46362 [details] simple testcase However accumulator is not needed to reproduce bug, it shown the idea of my usage of sigc++, thus you may safely remove it from source This testcase also contain one of possible workarounds which is activated if SIGC_WORKAROUND is defined. With whis workaround enabled you'll see what I expect from libsigc++
Created attachment 46496 [details] [review] Patch to temp_slot_list. This patch should allow recursive signals (except when appended past the end), and inserting of slots anywhere in the slot list.
Alternatively, but not ABI compatible, I could just add a pending_slots_ list to signal_base. Connect could check exec_count and add to the appropriate list. sweep would then also append the pending slots to the main slot list.
AFAIK, temp_slot_list was introduced to not allow invocation of slots connected to signal during emition. With this patch this fails, as with first (of two) invoked slot I can add slot right after and it will be invoked, so we could just remove temp_slot_list at all. I had hard thinking about all of this and didn't find any other possibility except copying slot_list to temp_slot_list. This will fixate slot list so any operations on it will not been seen during next emition. But this is expensive operation. If there are any other way to "fixate" slot list during only current emition - it would be better way, but I can't think on any. Other solution is to remove temp_slot_list at all and relay questions concerning operation on signal's slots during emition to user/programmer.
Oops "it will not been seen during next emition" should read as "it will not been seen _until_ next emition (recursive or not)" PS. Also by "other way" I mean less expensive one
The temp_slot_list's purpose was to prevent infinite recursion via connect, but If you're manipulating the slot_list directly and you want some form of recursion, I don't know that that needs prevented. There is nothing inherently wrong with recursion, just that it shouldn't happen by default. I thought I understood your report to claim you -wanted- to be able to have recursion, which would not be possible with the heavyweight method of copying the entire list. This patch (and my subsequent suggestion) solves my problem with unexpected recursion, while allowing the user to intelligently manipulate the slot_list (or unintelligently manipulate it and causing himself infinite recursion :) I think My Comment #3 is actually the solution, but I like providing ABI compatible solutions until the ABI can be broken.
Ok, I understand what you means - do not allow just connected (via connect()) slots to be invoked in current emition. But I thought it was not allow invokation of any just connected (via any possible interface) slots during emition until next one. Well, this patch allows me do what I want and I'm almost happy with it :) But it bring another problem - the same as in your's bug #166217 (infinite loops) but using slots() interface. I'll think more on this, and in case of success add comment/patch here, in case of failue it worth documenting it in its current state
I'm lost. Can someone summarize this, please? For instance, does this patch fix the problem? Does it introduce extra bugs/regressions? Should I commit it?
The patch fixes the problem (as well as maintaining the fix to bug #166217) with one caveat: If you append past the end of the slot_list during a signal emittion, that new slot will not be called until the next emittion (preventing recursive slot calls in this case). This is because we're only iterating to the placeholder slot instead of the end. Personally, I would use this patch as a non-ABI breaking solution until the ABI may be broken. Then the ideal solution would be to add a pending slot list, which would be used by connect during signal emittion. This would maintain the fix to bug #166217 and allow recursive slot calls even when a slot is inserted past the end of the slot list. I would be more than happy to provide a patch with the ideal solution, but I'm guessing the users would like an ABI backward compatible version also. P.S. I ran the patch against all the test programs and got lots of output from them, but nothing that told me decisively whether they passed or failed.
All is OK with this patch as I understand purpose of bug #166217. I already recompiled libsig++ with patch applied and removed hack from my sources. All works as expected now. However one little problem stay (if it is there at all) - documentation. Doc comment above temp_slot_list class remain from previuos implementatiion.
Committed. Thanks. Please submit a patch for that documentation.
Please.
Created attachment 49051 [details] [review] temp_slot_list doc update As requested, here is updated documentation.
Comitted. Thanks.