Rewrite event receiver tracking to allow safe receiver removal during event send, even when same number of receivers is added and removed. Closes #1642.

Source/Urho3D/Core/Context.cpp

@@ -31,6 +31,50 @@
 namespace Urho3D
 {
 
+void EventReceiverGroup::BeginSendEvent()
+{
+    ++inSend_;
+}
+
+void EventReceiverGroup::EndSendEvent()
+{
+    assert(inSend_ > 0);
+    --inSend_;
+
+    if (inSend_ == 0 && dirty_)
+    {
+        /// \todo Could be optimized by erase-swap, but this keeps the receiver order
+        for (unsigned i = receivers_.Size() - 1; i < receivers_.Size(); --i)
+        {
+            if (!receivers_[i])
+                receivers_.Erase(i);
+        }
+
+        dirty_ = false;
+    }
+}
+
+void EventReceiverGroup::Add(Object* object)
+{
+    if (object)
+        receivers_.Push(object);
+}
+
+void EventReceiverGroup::Remove(Object* object)
+{
+    if (inSend_ > 0)
+    {
+        PODVector<Object*>::Iterator i = receivers_.Find(object);
+        if (i != receivers_.End())
+        {
+            (*i) = 0;
+            dirty_ = true;
+        }
+    }
+    else
+        receivers_.Remove(object);
+}
+
 void RemoveNamedAttribute(HashMap<StringHash, Vector<AttributeInfo> >& attributes, StringHash objectType, const char* name)
 {
     HashMap<StringHash, Vector<AttributeInfo> >::Iterator i = attributes.Find(objectType);
@@ -243,23 +287,27 @@ AttributeInfo* Context::GetAttribute(StringHash objectType, const char* name)
 
 void Context::AddEventReceiver(Object* receiver, StringHash eventType)
 {
-    eventReceivers_[eventType].Insert(receiver);
+    eventReceivers_[eventType].Add(receiver);
 }
 
 void Context::AddEventReceiver(Object* receiver, Object* sender, StringHash eventType)
 {
-    specificEventReceivers_[sender][eventType].Insert(receiver);
+    specificEventReceivers_[sender][eventType].Add(receiver);
 }
 
 void Context::RemoveEventSender(Object* sender)
 {
-    HashMap<Object*, HashMap<StringHash, HashSet<Object*> > >::Iterator i = specificEventReceivers_.Find(sender);
+    HashMap<Object*, HashMap<StringHash, EventReceiverGroup > >::Iterator i = specificEventReceivers_.Find(sender);
     if (i != specificEventReceivers_.End())
     {
-        for (HashMap<StringHash, HashSet<Object*> >::Iterator j = i->second_.Begin(); j != i->second_.End(); ++j)
+        for (HashMap<StringHash, EventReceiverGroup>::Iterator j = i->second_.Begin(); j != i->second_.End(); ++j)
         {
-            for (HashSet<Object*>::Iterator k = j->second_.Begin(); k != j->second_.End(); ++k)
-                (*k)->RemoveEventSender(sender);
+            for (unsigned k = 0; k < j->second_.receivers_.Size(); ++k)
+            {
+                Object* receiver = j->second_.receivers_[k];
+                if (receiver)
+                    receiver->RemoveEventSender(sender);
+            }
         }
         specificEventReceivers_.Erase(i);
     }
@@ -267,16 +315,16 @@ void Context::RemoveEventSender(Object* sender)
 
 void Context::RemoveEventReceiver(Object* receiver, StringHash eventType)
 {
-    HashSet<Object*>* group = GetEventReceivers(eventType);
+    EventReceiverGroup* group = GetEventReceivers(eventType);
     if (group)
-        group->Erase(receiver);
+        group->Remove(receiver);
 }
 
 void Context::RemoveEventReceiver(Object* receiver, Object* sender, StringHash eventType)
 {
-    HashSet<Object*>* group = GetEventReceivers(sender, eventType);
+    EventReceiverGroup* group = GetEventReceivers(sender, eventType);
     if (group)
-        group->Erase(receiver);
+        group->Remove(receiver);
 }
 
 void Context::BeginSendEvent(Object* sender, StringHash eventType)

Source/Urho3D/Core/Context.h

@@ -29,6 +29,39 @@
 namespace Urho3D
 {
 
+/// Tracking structure for event receivers.
+class URHO3D_API EventReceiverGroup
+{
+public:
+    /// Construct.
+    EventReceiverGroup() :
+        inSend_(0),
+        dirty_(false)
+    {
+    }
+
+    /// Begin event send. When receivers are removed during send, group has to be cleaned up afterward.
+    void BeginSendEvent();
+
+    /// End event send. Clean up if necessary.
+    void EndSendEvent();
+
+    /// Add receiver. Same receiver must not be double-added!
+    void Add(Object* object);
+
+    /// Remove receiver. Leave holes during send, which requires later cleanup.
+    void Remove(Object* object);
+
+    /// Receivers. May contain holes during sending.
+    PODVector<Object*> receivers_;
+
+private:
+    /// "In send" recursion counter.
+    unsigned inSend_;
+    /// Cleanup required flag.
+    bool dirty_;
+};
+
 /// Urho3D execution context. Provides access to subsystems, object factories and attributes, and event receivers.
 class URHO3D_API Context : public RefCounted
 {
@@ -135,12 +168,12 @@ public:
     const HashMap<StringHash, Vector<AttributeInfo> >& GetAllAttributes() const { return attributes_; }
 
     /// Return event receivers for a sender and event type, or null if they do not exist.
-    HashSet<Object*>* GetEventReceivers(Object* sender, StringHash eventType)
+    EventReceiverGroup* GetEventReceivers(Object* sender, StringHash eventType)
     {
-        HashMap<Object*, HashMap<StringHash, HashSet<Object*> > >::Iterator i = specificEventReceivers_.Find(sender);
+        HashMap<Object*, HashMap<StringHash, EventReceiverGroup> >::Iterator i = specificEventReceivers_.Find(sender);
         if (i != specificEventReceivers_.End())
         {
-            HashMap<StringHash, HashSet<Object*> >::Iterator j = i->second_.Find(eventType);
+            HashMap<StringHash, EventReceiverGroup>::Iterator j = i->second_.Find(eventType);
             return j != i->second_.End() ? &j->second_ : 0;
         }
         else
@@ -148,9 +181,9 @@ public:
     }
 
     /// Return event receivers for an event type, or null if they do not exist.
-    HashSet<Object*>* GetEventReceivers(StringHash eventType)
+    EventReceiverGroup* GetEventReceivers(StringHash eventType)
     {
-        HashMap<StringHash, HashSet<Object*> >::Iterator i = eventReceivers_.Find(eventType);
+        HashMap<StringHash, EventReceiverGroup>::Iterator i = eventReceivers_.Find(eventType);
         return i != eventReceivers_.End() ? &i->second_ : 0;
     }
 
@@ -182,9 +215,9 @@ private:
     /// Network replication attribute descriptions per object type.
     HashMap<StringHash, Vector<AttributeInfo> > networkAttributes_;
     /// Event receivers for non-specific events.
-    HashMap<StringHash, HashSet<Object*> > eventReceivers_;
+    HashMap<StringHash, EventReceiverGroup > eventReceivers_;
     /// Event receivers for specific senders' events.
-    HashMap<Object*, HashMap<StringHash, HashSet<Object*> > > specificEventReceivers_;
+    HashMap<Object*, HashMap<StringHash, EventReceiverGroup > > specificEventReceivers_;
     /// Event sender stack.
     PODVector<Object*> eventSenders_;
     /// Event data stack.

Source/Urho3D/Core/Object.cpp

@@ -143,11 +143,15 @@ void Object::SubscribeToEvent(StringHash eventType, EventHandler* handler)
     EventHandler* previous;
     EventHandler* oldHandler = FindSpecificEventHandler(0, eventType, &previous);
     if (oldHandler)
+    {
         eventHandlers_.Erase(oldHandler, previous);
-
-    eventHandlers_.InsertFront(handler);
-
-    context_->AddEventReceiver(this, eventType);
+        eventHandlers_.InsertFront(handler);
+    }
+    else
+    {
+        eventHandlers_.InsertFront(handler);
+        context_->AddEventReceiver(this, eventType);
+    }
 }
 
 void Object::SubscribeToEvent(Object* sender, StringHash eventType, EventHandler* handler)
@@ -164,11 +168,15 @@ void Object::SubscribeToEvent(Object* sender, StringHash eventType, EventHandler
     EventHandler* previous;
     EventHandler* oldHandler = FindSpecificEventHandler(sender, eventType, &previous);
     if (oldHandler)
+    {
         eventHandlers_.Erase(oldHandler, previous);
-
-    eventHandlers_.InsertFront(handler);
-
-    context_->AddEventReceiver(this, sender, eventType);
+        eventHandlers_.InsertFront(handler);
+    }
+    else
+    {
+        eventHandlers_.InsertFront(handler);
+        context_->AddEventReceiver(this, sender, eventType);
+    }
 }
 
 #if URHO3D_CXX11
@@ -301,90 +309,79 @@ void Object::SendEvent(StringHash eventType, VariantMap& eventData)
     context->BeginSendEvent(this, eventType);
 
     // Check first the specific event receivers
-    const HashSet<Object*>* group = context->GetEventReceivers(this, eventType);
+    EventReceiverGroup* group = context->GetEventReceivers(this, eventType);
     if (group)
     {
-        for (HashSet<Object*>::ConstIterator i = group->Begin(); i != group->End();)
+        group->BeginSendEvent();
+
+        for (unsigned i = 0; i < group->receivers_.Size(); ++i)
         {
-            HashSet<Object*>::ConstIterator current = i++;
-            Object* receiver = *current;
-            Object* next = 0;
-            if (i != group->End())
-                next = *i;
+            Object* receiver = group->receivers_[i];
+            // Holes may exist if receivers removed during send
+            if (!receiver)
+                continue;
 
-            unsigned oldSize = group->Size();
             receiver->OnEvent(this, eventType, eventData);
 
             // If self has been destroyed as a result of event handling, exit
             if (self.Expired())
             {
+                group->EndSendEvent();
                 context->EndSendEvent();
                 return;
             }
 
-            // If group has changed size during iteration (removed/added subscribers) try to recover
-            /// \todo This is not entirely foolproof, as a subscriber could have been added to make up for the removed one
-            if (group->Size() != oldSize)
-                i = group->Find(next);
-
             processed.Insert(receiver);
         }
+
+        group->EndSendEvent();
     }
 
     // Then the non-specific receivers
     group = context->GetEventReceivers(eventType);
     if (group)
     {
+        group->BeginSendEvent();
+
         if (processed.Empty())
         {
-            for (HashSet<Object*>::ConstIterator i = group->Begin(); i != group->End();)
+            for (unsigned i = 0; i < group->receivers_.Size(); ++i)
             {
-                HashSet<Object*>::ConstIterator current = i++;
-                Object* receiver = *current;
-                Object* next = 0;
-                if (i != group->End())
-                    next = *i;
+                Object* receiver = group->receivers_[i];
+                if (!receiver)
+                    continue;
 
-                unsigned oldSize = group->Size();
                 receiver->OnEvent(this, eventType, eventData);
 
                 if (self.Expired())
                 {
+                    group->EndSendEvent();
                     context->EndSendEvent();
                     return;
                 }
-
-                if (group->Size() != oldSize)
-                    i = group->Find(next);
             }
         }
         else
         {
             // If there were specific receivers, check that the event is not sent doubly to them
-            for (HashSet<Object*>::ConstIterator i = group->Begin(); i != group->End();)
+            for (unsigned i = 0; i < group->receivers_.Size(); ++i)
             {
-                HashSet<Object*>::ConstIterator current = i++;
-                Object* receiver = *current;
-                Object* next = 0;
-                if (i != group->End())
-                    next = *i;
-
-                if (!processed.Contains(receiver))
-                {
-                    unsigned oldSize = group->Size();
-                    receiver->OnEvent(this, eventType, eventData);
+                Object* receiver = group->receivers_[i];
+                if (!receiver || processed.Contains(receiver))
+                    continue;
 
-                    if (self.Expired())
-                    {
-                        context->EndSendEvent();
-                        return;
-                    }
+                receiver->OnEvent(this, eventType, eventData);
 
-                    if (group->Size() != oldSize)
-                        i = group->Find(next);
+                if (self.Expired())
+                {
+                    group->EndSendEvent();
+                    context->EndSendEvent();
+                    return;
                 }
             }
         }
+
+        group->EndSendEvent();
     }
 
     context->EndSendEvent();

Source/Urho3D/Engine/Console.cpp

@@ -278,13 +278,17 @@ bool Console::PopulateInterpreter()
 {
     interpreters_->RemoveAllItems();
 
-    HashSet<Object*>* receivers = context_->GetEventReceivers(E_CONSOLECOMMAND);
-    if (!receivers || receivers->Empty())
+    EventReceiverGroup* group = context_->GetEventReceivers(E_CONSOLECOMMAND);
+    if (!group || group->receivers_.Empty())
         return false;
 
     Vector<String> names;
-    for (HashSet<Object*>::ConstIterator iter = receivers->Begin(); iter != receivers->End(); ++iter)
-        names.Push((*iter)->GetTypeName());
+    for (unsigned i = 0; i < group->receivers_.Size(); ++i)
+    {
+        Object* receiver = group->receivers_[i];
+        if (receiver)
+            names.Push(receiver->GetTypeName());
+    }
     Sort(names.Begin(), names.End());
 
     unsigned selection = M_MAX_UNSIGNED;