The signal that destroys its owner

Recommended Posts

So I've been using this signal/slot-library for quite some time now: https://github.com/pbhogan/Signals

One rather "common" pattern I encountered, is a signal that destroys its owning class:

class Widget
{
core::Signal<> SigClicked;
}

void onCloseWindow(void)
{
delete &widget;
}

widget.SigClicked.Connect(&onCloseWindow);
widget.SigClicked(); // ...

Now this is going a bit into the details of how signals is implemented - internally it stores an std::set, which in the operator() is iterated via a for-loop:

void Signal::operator()() const
{
for(auto& delegate : delegate_list)
{
delegate.Call();
}
}

Now can you imagine what happens when any of the delegates destroys the object that owns the Signal? Obviously this will result in the destructor of the Signal being called, after which it will resume to iterate over the list - crash, in the best case

I've encountered multiple strategies to combat this - the most obvious that I know is to simply delay the delete/destroy-operator, ie. by performing it in the next tick-step. This can overcomplicate things though, so I searched for a simpler solution-until I finally found one:

void Signal::EmitSafe() const
{
const auto temp_list = delegate_list;
for(auto& delegate : temp_list)
{
delegate.Call();
}
}

Now I'm not sure if thats a coding-horror, or an actual elegant solution for this problem Its not universal though, since due to the overhead of copying the list everytime, I only use it where I can expect something like this to happen.

Did you ever have fun with objects trying to delete themselves in a similar fasion? I'd also like to hear some suggestions of what you'd do to solve that

Share on other sites
On 6/18/2017 at 2:17 PM, Juliean said:

Did you ever have fun with objects trying to delete themselves in a similar fasion?

I've seen many variations of delayed destruction.

Managing the life cycle of game objects is sometimes tricky.  For objects in the game world there are typically states of {created but no content}, {content but not in world}, {in world and inactive}, {in world and inactive}.  In that case a call to destroy would remove the object from the world and mark it as inactive, but the actual destruction would take place outside the main update and processing loop.

For objects like you describe, I've seen that pattern quite often. Make a second list of items to be removed, then process the dead list at a more appropriate time.

Another option if items are often in flux is to add a flag to indicate if the node is live or dead. As the collection is processed each item is tested for life.  To remove an item you simply mark it as dead. When another item is added it will replace the dead one. That eliminates the need to resize, potentially invalidate the container, or move items around.

Share on other sites

Come to think of it, I'm already doing what you describe for my entities/game objects, at least. Having a game-object destroyed while its script is running/issuing the destroy command was something I encountered quite early, and alleviated by introducing a destroyed-queue like you described.

For widgets (which is my primary source of pain regarding this topic), I never though of doing that. There's a certain additional level of complication that I didn't make clear yet, though:

- Widgets are generally user-managed. Meaning, they are mostly created & stored by a std::unique_ptr in the user code. I used some managed storage before, and it caused different problems, so I ended up this way. On the other hand, this means that its neigh impossible to introduce a dead-widget queue to solve the problem. For example, pseudo-code for a asset-viewer that allows to tab between different assets:

class AssetView
{
using TabMap = std::unordered_map<Asset*, std::unique_ptr<AssetTabView>>;

private:

void OnCloseTab(Asset* pAsset) // isued by TabBar::TabItem::SigClose
{
m_pTabs->RemoveTab(pAsset); // oups

m_mTabs.erase(pAsset);
}

std::unique_ptr<TabBar> m_pTabs;
TabMap m_mTabs;
}

This at the very least means that TabBar has to implement its own dead-widget queue, or I have to implement the delayed-destroy in the user code, like in this "AssetView".

- This, unlike the example with the game-objects, can also happen when a slot gets removed from/added to the signal while iterating, as this will also invalidate iterators. This happens more often then you'd probably imagine - ie. when reloading the generated shader code of my "material"-system, this sometimes forces new material-instances to be generated, which in turn will have to register with the "SigReload" of its material-class. Now sure, another thing where I could just delay the initialization, but quite frankly, this happens so frequently that it becomes a huge pain in the ass, having to delay everything without much added benefit (IMHO; as opposed to game-objects where it makes quite much sense).

Share on other sites

If it's a common pattern,  then make "i just destroyed myself" a core part of the signalling system, so your loop can terminate correctly.

Share on other sites
14 hours ago, Hodgman said:

If it's a common pattern,  then make "i just destroyed myself" a core part of the signalling system, so your loop can terminate correctly.

You mean something like this?

class Signal
{
~Signal(void)
{
}

void Emit(void) const
{
for(auto& delegate : m_list)
{
delegate();

break;
}
}

private:

}

Seems a bit hacky & unsafe though - I imagine you had something different in mind, but since signals are just standalone classes that are composed into other classes, there's currently no other (easy) way to let the loop know.

I'm also not sure if thats even a good idea. Its usually important that all slots are correctly notified, even if one of them ends up terminating the signal - otherwise it can lead to invalid state/further crashes. Especially since the order of the slots is not guaranteed, this can easily lead to undefined behaviour, based on which slot is called first on any given time. I quess in that sense, creating a copy of the list is a necessary "evil".

Share on other sites

I think Hodgman is more talking along the lines of  the of the Win32 message loop; GetMessage() and PeekMessage() return an error code/exit code so that the message loop knows when the final 'quit' message has been received.  You would either need Delegate() to return an error/exit code, or have a special CheckDelegate()/ExitDelegate() function that returns an error/exit code if the next delegate function is the special 'quit' message.  That way the message loops knows when to stop processing messages.

If all the delegates have to be executed, perhaps execute just the delegates that are non-destroying first, then all the destroying one?  Something like this comes to mind:

// returns true if the delegate is an 'exit' message for the given object
bool IsExit(const Delegate&, const Signal*);

void Signal::operator()() const {
for(auto& delegate : delegate_list) {
if (!IsExit(delegate, this)) delegate.Call();
}
for(auto& delegate : delegate_list) {
if (IsExit(delegate, this)) {
delegate.Call();
return;
}
}
}

Or perhaps something like:

void Signal::operator()() const {

bool has_exit_signal = false;

for(auto& delegate : delegate_list) {
if (IsExit(delegate, this)) has_exit_signal = true;
else delegate.Call();
}

if (has_exit_signal) {
// write special code to kill object
// delete this;
// DestroyObject(this);
// depending on how the objects are created/stored would change what goes here...
}
}

Edited by Ryan_001

Create an account

Register a new account