Fix possible race condition in user-defined thread-safety mechanism

src/core/agent/agent.h

@@ -151,11 +151,23 @@ class Agent {
 
   /// If the thread-safety mechanism is set to user-specified this function
   /// will be called before the operations are executed for this agent.\n
-  /// Subclasses define the critical region by adding the uids of all
+  /// Subclasses define the critical region by adding the AgentPointers of all
   /// agents that must not be processed in parallel. \n
-  /// Don't forget to add the uid of the current agent.\n
+  /// Don't forget to add the current agent.\n
+  /// Here an example from NeuronSoma.\n
+  ///
+  ///     void NeuronSoma::CriticalRegion(std::vector<AgentPointer<>>* aptrs) {
+  ///       aptrs->reserve(daughters_.size() + 1);
+  ///       aptrs->push_back(Agent::GetAgentPtr<>());
+  ///       for (auto& daughter : daughters_) {
+  ///         aptrs->push_back(daughter);
+  ///       }
+  ///     }
+  ///
   /// \see `Param::thread_safety_mechanism`
-  virtual void CriticalRegion(std::vector<AgentUid>* uids) {}
+  /// \see `AgentPointer`
+  /// \see `NeuronSoma::CriticalRegion`
+  virtual void CriticalRegion(std::vector<AgentPointer<>>* aptrs) {}
 
   uint32_t GetBoxIdx() const;
 

src/core/agent/agent_pointer.h

@@ -21,7 +21,7 @@
 #include <type_traits>
 
 #include "core/agent/agent_uid.h"
-#include "core/execution_context/in_place_exec_ctxt.h"
+#include "core/execution_context/execution_context.h"
 #include "core/simulation.h"
 #include "core/util/root.h"
 
@@ -37,7 +37,7 @@ class Agent;
 /// the type returned by `Get` and can therefore inline the code from the callee
 /// and perform optimizations.
 /// @tparam TAgent agent type
-template <typename TAgent>
+template <typename TAgent = Agent>
 class AgentPointer {
  public:
   explicit AgentPointer(const AgentUid& uid) : uid_(uid) {}
@@ -106,8 +106,12 @@ class AgentPointer {
 
   const TAgent& operator*() const { return *(this->operator->()); }
 
+  bool operator<(const AgentPointer& other) const { return uid_ < other.uid_; }
+
   operator bool() const { return *this != nullptr; }  // NOLINT
 
+  operator AgentPointer<Agent>() const { return AgentPointer<Agent>(uid_); }
+
   TAgent* Get() { return this->operator->(); }
 
   const TAgent* Get() const { return this->operator->(); }

src/core/execution_context/execution_context.h

@@ -18,7 +18,9 @@
 #include <utility>
 #include <vector>
 
+#include "core/agent/agent_handle.h"
 #include "core/agent/agent_uid.h"
+#include "core/container/math_array.h"
 #include "core/functor.h"
 #include "core/operation/operation.h"
 

src/core/execution_context/in_place_exec_ctxt.cc

@@ -157,27 +157,40 @@ void InPlaceExecutionContext::Execute(
       critical_region_2_.clear();
       locks_.clear();
       agent->CriticalRegion(&critical_region_);
+      // Sort such that the locks further down are acquired in a sorted order
+      // This technique avoids deadlocks.
       std::sort(critical_region_.begin(), critical_region_.end());
-      for (auto uid : critical_region_) {
-        locks_.push_back(GetAgent(uid)->GetLock());
+      // Remove all AgentPointers which correspond to a nullptr.
+      while (critical_region_.size() && critical_region_.back() == nullptr) {
+        critical_region_.pop_back();
+      }
+      // Remove all duplicate entries
+      critical_region_.erase(
+          std::unique(critical_region_.begin(), critical_region_.end()),
+          critical_region_.end());
+      for (auto aptr : critical_region_) {
+        locks_.push_back(aptr->GetLock());
       }
       for (auto* l : locks_) {
         l->lock();
       }
       agent->CriticalRegion(&critical_region_2_);
+      // Sort such that the locks further down are acquired in a sorted order
+      // This technique avoids deadlocks.
       std::sort(critical_region_2_.begin(), critical_region_2_.end());
-      bool same = true;
-      if (critical_region_.size() == critical_region_2_.size()) {
-        for (size_t i = 0; i < critical_region_.size(); ++i) {
-          if (critical_region_[i] != critical_region_2_[i]) {
-            same = false;
-            break;
-          }
-        }
-      } else {
-        same = false;
+      // Remove all AgentPointers which correspond to a nullptr.
+      while (critical_region_2_.size() &&
+             critical_region_2_.back() == nullptr) {
+        critical_region_2_.pop_back();
       }
-      if (same) {
+      // Remove all duplicate entries
+      critical_region_2_.erase(
+          std::unique(critical_region_2_.begin(), critical_region_2_.end()),
+          critical_region_2_.end());
+      // if the critical regions are not the same, then another thread
+      // changed it before the locks were acquired. In this case we have to
+      // try again. Otherwise we can leave the while loop.
+      if (critical_region_ == critical_region_2_) {
         break;
       }
       for (auto* l : locks_) {

src/core/execution_context/in_place_exec_ctxt.h

@@ -22,6 +22,7 @@
 #include <vector>
 
 #include "core/agent/agent_handle.h"
+#include "core/agent/agent_pointer.h"
 #include "core/agent/agent_uid.h"
 #include "core/container/agent_uid_map.h"
 #include "core/container/math_array.h"
@@ -140,13 +141,6 @@ class InPlaceExecutionContext : public ExecutionContext {
 
   ThreadInfo* tinfo_;
 
-  /// Contains unique ids of agents that will be removed at the end of each
-  /// iteration. AgentUids are separated by numa node.
-  std::vector<AgentUid> remove_;
-  std::vector<AgentUid> critical_region_;
-  std::vector<AgentUid> critical_region_2_;
-  std::vector<Spinlock*> locks_;
-
   /// Pointer to new agents
   std::vector<Agent*> new_agents_;
 
@@ -169,6 +163,17 @@ class InPlaceExecutionContext : public ExecutionContext {
 
   virtual void RemoveAgentsFromRm(
       const std::vector<ExecutionContext*>& all_exec_ctxts);
+
+ private:
+  /// Contains unique ids of agents that will be removed at the end of each
+  /// iteration. AgentUids are separated by numa node.
+  std::vector<AgentUid> remove_;
+  /// Used to determine which agents must not be updated from different threads.
+  std::vector<AgentPointer<>> critical_region_;
+  /// Used to determine which agents must not be updated from different threads.
+  std::vector<AgentPointer<>> critical_region_2_;
+
+  std::vector<Spinlock*> locks_;
 };
 
 }  // namespace bdm

src/core/type_index.h

@@ -17,6 +17,7 @@
 
 #include <vector>
 #include "core/agent/agent.h"
+#include "core/container/agent_uid_map.h"
 #include "core/container/flatmap.h"
 
 class TClass;

src/neuroscience/neurite_element.cc

@@ -119,15 +119,15 @@ void NeuriteElement::Update(const NewAgentEvent& event) {
   }
 }
 
-void NeuriteElement::CriticalRegion(std::vector<AgentUid>* uids) {
-  uids->reserve(4);
-  uids->push_back(GetUid());
-  uids->push_back(mother_.GetUid());
-  if (daughter_left_ != nullptr) {
-    uids->push_back(daughter_left_.GetUid());
+void NeuriteElement::CriticalRegion(std::vector<AgentPointer<>>* aptrs) {
+  aptrs->reserve(4);
+  aptrs->push_back(GetAgentPtr<>());
+  aptrs->push_back(mother_);
+  if (daughter_left_) {
+    aptrs->push_back(daughter_left_);
   }
-  if (daughter_right_ != nullptr) {
-    uids->push_back(daughter_right_.GetUid());
+  if (daughter_right_) {
+    aptrs->push_back(daughter_right_);
   }
 }
 

src/neuroscience/neurite_element.h

@@ -67,7 +67,7 @@ class NeuriteElement : public Agent, public NeuronOrNeurite {
 
   Spinlock* GetLock() override { return Base::GetLock(); }
 
-  void CriticalRegion(std::vector<AgentUid>* uids) override;
+  void CriticalRegion(std::vector<AgentPointer<>>* aptrs) override;
 
   Shape GetShape() const override { return Shape::kCylinder; }
 

src/neuroscience/neuron_soma.cc

@@ -66,11 +66,11 @@ void NeuronSoma::Update(const NewAgentEvent& event) {
   // do nothing for CellDivisionEvent or others
 }
 
-void NeuronSoma::CriticalRegion(std::vector<AgentUid>* uids) {
-  uids->reserve(daughters_.size() + 1);
-  uids->push_back(Agent::GetUid());
+void NeuronSoma::CriticalRegion(std::vector<AgentPointer<>>* aptrs) {
+  aptrs->reserve(daughters_.size() + 1);
+  aptrs->push_back(Agent::GetAgentPtr<>());
   for (auto& daughter : daughters_) {
-    uids->push_back(daughter.GetUid());
+    aptrs->push_back(daughter);
   }
 }
 

src/neuroscience/neuron_soma.h

@@ -61,7 +61,7 @@ class NeuronSoma : public Cell, public NeuronOrNeurite {
 
   Spinlock* GetLock() override { return Base::GetLock(); }
 
-  void CriticalRegion(std::vector<AgentUid>* uids) override;
+  void CriticalRegion(std::vector<AgentPointer<>>* aptrs) override;
 
   // ***************************************************************************
   //      METHODS FOR NEURON TREE STRUCTURE *