GNOME Bugzilla – Bug 738602
It is not possible to get a sigc::slot to release bound arguments
Last modified: 2015-10-11 13:22:29 UTC
Created attachment 288633 [details] Sample code that tries to clear a sigc::slot If I use sigc::bind to bind an object to a sigc::slot, there does not appear to be any way to destroy that bound object without destroying the slot. I've tried: slot.disconnect(); and slot = SlotType(); but neither works. sigc::slot does not appear to have clear() or reset() methods. (Also see bug 311057.) Sample code is attached that demonstrates this problem. I'd expect that the Test object would be destroyed either when disconnecting the slot or when reassigning the slot, but it isn't destroyed until the slot itself is destroyed.
Created attachment 288687 [details] Sample code, version 2 Yes, you're right. slot::disconnect() just invalidates the slot, it does not destroy the stored functor object. Assigning an empty slot does the same. Assigning a non-empty slot destroys the previously stored functor object. See the attached modified version of your test case. I don't know why the stored functor is kept for as long as possible in the slot. Is it a problem for you? When do you want it to be destroyed?
My expectation is that the functor would be destroyed once it's been invalidated. (And if disconnect() doesn't do it, I'd at least expect that assigning an empty slot over it would.) This is causing problems for us because objects bound to slots are living much longer than they should and aren't releasing resources that they own. (We're currently working around it by heap-allocating the slots and deleting them.) This also seems inconsistent with the behavior of slot invalidation when involved sigc::trackables are destroyed. For example, if I bind a sigc::ref of a sigc::trackable to a slot, destroying the sigc::trackable invalidates the slot *and* destroys any other bound arguments. I too don't see any reason to keep the stored functor around when the slot is disconnected. It's not as if a slot can be un-disconnected.
The following table shows what happens with a sigc::slot in different situations, and (in parentheses) what I think should happen. ------- Event --------------- ------------- Result --------------- Disconnected Invalidated Destroyed Bound trackable is destroyed yes yes yes slot_base::disconnect() called yes yes(no) no Empty slot assigned yes(no) yes no(yes) Non-empty slot assigned no yes yes "Disconnected" means disconnect from parent, if there is one, else do nothing. The parent can be a sigc::signal or something else, such as a Glib::SignalProxyConnectionNode. I don't understand why invalidation and destruction are separated. An invalidated slot can't be made valid again. It can be destroyed immediately. A disconnected, but still valid, slot can perhaps be re-connected. When the parent is a sigc::signal, it's not possible. sigc::signal::connect() takes a copy of the slot, and deletes that copy when it's disconnected. With another parent, reconnection might be possible. IMO there should be no difference between assigning an empty slot and a non-empty slot. (Except that nothing shall happen when the function call operator of an empty slot is invoked.) I don't want to change everything now to make it work exactly like I think it should. That would break API. The documentation of slot_base::disconnect() explicitly says that it invalidates the slot. Keeping an empty slot connected can be problematic (but probably not impossible) with the present internal structure of a slot. I'd prefer not to do it now. Although unlikely, it might break some user application. What I want to do now is to make assignment of an empty slot destroy the old slot. Would it be ok if I do only that? Making slot_base::disconnect() destroy the slot is reasonable as long as it invalidates the slot, but it would make disconnect() even more different from how I think it ought to behave. I'm reluctant to do it, but it's possible that you can persuade me.
(In reply to comment #3) > A disconnected, but still valid, slot can perhaps be re-connected. When the > parent is a sigc::signal, it's not possible. sigc::signal::connect() takes a > copy of the slot, and deletes that copy when it's disconnected. > With another parent, reconnection might be possible. If someone wants to temporarily suppress a slot and to then re-enable it, using sigc::slot_base::block()/unblock() would be more appropriate. I don't see a need to want to reconnect a slot. Since we agree that it doesn't make sense to make an invalid slot valid again and since sigc::slot_base::disconnect() is documented to "invalidate the slot", I don't think anyone should expect to be able to do something useful with a slot after calling disconnect(). That is, I have no concern about potential desire to reconnect a slot since there currently is no way to disconnect a slot without invalidating it. > IMO there should be no difference between assigning an empty slot and a > non-empty slot. (Except that nothing shall happen when the function call > operator of an empty slot is invoked.) Agreed. > I don't want to change everything now to make it work exactly like I think it > should. That would break API. The documentation of slot_base::disconnect() > explicitly says that it invalidates the slot. Keeping an empty slot connected > can be problematic (but probably not impossible) with the present internal > structure of a slot. I'd prefer not to do it now. Although unlikely, it might > break some user application. How about breaking API for a future 2.5.0 release? > What I want to do now is to make assignment of an empty slot destroy the old > slot. Would it be ok if I do only that? It would at least be an improvement over the current state, so that's acceptable to me (although not ideal). > Making slot_base::disconnect() destroy the slot is reasonable as long as it > invalidates the slot, but it would make disconnect() even more different from > how I think it ought to behave. I'm reluctant to do it, but it's possible that > you can persuade me. I personally don't think there should be subtle differences between disconnection and invalidation, and I don't see the need for such a distinction. sigc::slot_base::disconnect()'s documentation implies that they're equivalent, and that's the expectation that existing consumers have.
(In reply to comment #4) > If someone wants to temporarily suppress a slot and to then re-enable it, using > sigc::slot_base::block()/unblock() would be more appropriate. What I had in mind was rather that someone wants to disconnect a slot from one parent, and then connect it to another parent. It's not possible now, and I guess the same result can be achieved by copying the slot, and connecting different copies to different parents. > How about breaking API for a future 2.5.0 release? API and ABI must not be broken until a 3.0.0 release. (At least in theory. There was a small API break in 2.3.2 and 2.4.0. See bug 724496 or the NEWS file.) >> Making slot_base::disconnect() destroy the slot is reasonable as long as it >> invalidates the slot. I've had a second (or even third or fourth) thought about this. It's risky to let disconnect() destroy the slot. What makes it risky is that a slot can disconnect itself in its function call operator. If it's connected to a sigc::signal, that whould be when the signal is emitted. If slot_base::disconnect() would destroy the slot, then calling disconnect() from the slot itself would be somewhat like 'delete this'. You can do it at the end of a member function, but don't you dare to access any part of the slot's data afterwards! Such a restriction on disconnect() would probably have been acceptable, if it had been specified from the beginning. Introducing it now may break some user code. Summary ------- 1. I don't want to change disconnect(). It's too risky. 2. I will make assignment of an empty slot destroy the old slot. 3. Shall a connected slot be disconnected, when an empty slot is assigned to it? It is now. Do you want that changed? Is it important? It's not documented that assignment works like that. A change would perhaps not count as an API break. Such a change would have been trivial, if cleanup_ and parent_ had been members of slot_base instead of members of slot_rep.
(In reply to comment #5) > It's risky to > let disconnect() destroy the slot. What makes it risky is that a slot can > disconnect itself in its function call operator. If it's connected to a > sigc::signal, that whould be when the signal is emitted. Can you provide an example of how a slot could disconnect itself? In general, one approach to deal with potential self-destruction is to use reference-counting and to acquire an extra, temporary reference before calling the potentially self-destructing function. I don't know if that applies here, though, and maybe it's not really feasible. > Summary > ------- > 1. I don't want to change disconnect(). It's too risky. > > 2. I will make assignment of an empty slot destroy the old slot. Okay. > 3. Shall a connected slot be disconnected, when an empty slot is assigned to > it? It is now. Do you want that changed? Is it important? > It's not documented that assignment works like that. A change would perhaps > not count as an API break. Such a change would have been trivial, if > cleanup_ and parent_ had been members of slot_base instead of members of > slot_rep. I think it makes sense for an empty slot to be disconnected, so that aspect of the current behavior seems okay to me.
> Can you provide an example of how a slot could disconnect itself? It's done in one of libsigc++'s test cases: https://git.gnome.org/browse/libsigc++2/tree/tests/test_disconnect_during_emit.cc In this example the slot function is extremely simple. I'm sure it will not crash, if disconnect() destroys the slot. It takes a more complicated example to get a crash or any other bad behavior. I have pushed a patch that fixes the slot_base::operator=(). https://git.gnome.org/browse/libsigc++2/commit/?id=91d2ffa5d54fdae1fc8d599f9a756932640e0ad5
Updated version of the table in comment 3: ------- Event --------------- ------------- Result --------------- Disconnected Invalidated Destroyed Bound trackable is destroyed yes yes yes slot_base::disconnect() called yes yes (1) no (2) Empty slot assigned yes (3) yes yes (4) Non-empty slot assigned no yes yes (1) I've changed my mind since I wrote comment 3. This behaviour is ok. It's documented, and as a fix of bug 311057 even a slot which has never been connected is invalidated by a call to disconnect(). (2) Acceptable but not ideal. As explained in comment 5, it would be risky to let disconnect() destroy the slot. (3) Acceptable but not ideal. Because parent_ is a member of slot_rep, an empty slot can't have a parent. (4) Changed as a fix of this bug.