Merge branch 'master-mutextes' into 'master'

Thread-safety patches

See merge request !109

GeoModelCore/GeoModelKernel/GeoModelKernel/GeoAlignableTransform.h

@@ -7,6 +7,7 @@
 
 #include "GeoModelKernel/GeoTransform.h"
 #include <vector>
+#include <mutex>
 
 class GeoVAlignmentStore;
 
@@ -45,6 +46,10 @@ class GeoAlignableTransform final : public GeoTransform
   // the memory is unallocated.
   GeoTrf::Transform3D* m_delta;
 
+  // We need to protext m_delta with a mutex in order to avoid
+  // memory corruption in multithreaded applications
+  mutable std::mutex m_deltaMutex;
+  
   // A list of parents who use this alignable target.  They
   // must all be notified when the alignment changes!
   std::vector<GeoGraphNode*>  m_parentList;

GeoModelCore/GeoModelKernel/GeoModelKernel/GeoVFullPhysVol.h

@@ -7,6 +7,7 @@
 
 #include "GeoModelKernel/GeoVPhysVol.h"
 #include "GeoModelKernel/GeoAbsPositionInfo.h"
+#include <mutex>
 
 class GeoVAlignmentStore;
 
@@ -51,6 +52,11 @@ class GeoVFullPhysVol : public GeoVPhysVol
  protected:
   virtual ~GeoVFullPhysVol() override;
 
+  /// Mutex serving dual purpose:
+  ///  1. To protect the absolute position info
+  ///  2. To protect m_daughters container in the derived GeoFullPhysVol
+  mutable std::mutex m_mutex;
+
  private:
   /// The absolute name of this volume.
   std::string m_absName;

GeoModelCore/GeoModelKernel/src/GeoAlignableTransform.cxx

@@ -29,20 +29,29 @@ __attribute__ ((flatten))
 #endif
 GeoTrf::Transform3D GeoAlignableTransform::getTransform(const GeoVAlignmentStore* store) const
 {
-  const GeoTrf::Transform3D* delta = (store==nullptr ? m_delta : store->getDelta(this));
-  return GeoTransform::getTransform(nullptr) * (delta==nullptr ? GeoTrf::Transform3D(GeoTrf::Transform3D::Identity()) : *delta);
+  if(store) {
+    const GeoTrf::Transform3D* delta = store->getDelta(this);
+    return GeoTransform::getTransform(nullptr) * (delta==nullptr ? GeoTrf::Transform3D(GeoTrf::Transform3D::Identity()) : *delta);
+  }
+  else {
+    std::scoped_lock<std::mutex> guard(m_deltaMutex);
+    return GeoTransform::getTransform(nullptr) * (m_delta==nullptr ? GeoTrf::Transform3D(GeoTrf::Transform3D::Identity()) : *m_delta);
+  }
 }
 
 void GeoAlignableTransform::setDelta (const GeoTrf::Transform3D& delta, GeoVAlignmentStore* store)
 {
   if(store==nullptr) {
-    if(m_delta && (m_delta->isApprox(delta))) return;
-    
-    if(m_delta) {
-      (*m_delta) = delta;
-    }
-    else {
-      m_delta = new GeoTrf::Transform3D(delta);
+    {
+      std::scoped_lock<std::mutex> guard(m_deltaMutex);
+      if(m_delta && (m_delta->isApprox(delta))) return;
+      
+      if(m_delta) {
+	(*m_delta) = delta;
+      }
+      else {
+	m_delta = new GeoTrf::Transform3D(delta);
+      }
     }
 
     std::set<GeoGraphNode*> uniqueParents;
@@ -63,9 +72,11 @@ void GeoAlignableTransform::clearDelta(GeoVAlignmentStore* store)
 {
   // Does not make sence to clear deltas inside Alignment Store
   if(store!=nullptr) return;
-
-  delete m_delta;
-  m_delta = nullptr;
+  {
+    std::scoped_lock<std::mutex> guard(m_deltaMutex);
+    delete m_delta;
+    m_delta = nullptr;
+  }
   
   std::set<GeoGraphNode*> uniqueParents;
   for(GeoGraphNode* parent : m_parentList) {

GeoModelCore/GeoModelKernel/src/GeoFullPhysVol.cxx

@@ -26,7 +26,7 @@ GeoFullPhysVol::~GeoFullPhysVol()
 void GeoFullPhysVol::add(GeoGraphNode* graphNode)
 {
   if(m_cloneOrigin) throw std::runtime_error("Attempt to modify contents of a cloned FPV");
-
+  std::scoped_lock<std::mutex> guard(m_mutex);
   m_daughters.push_back(graphNode);
   graphNode->ref();
   graphNode->dockTo(this);
@@ -235,15 +235,18 @@ GeoTrf::Transform3D GeoFullPhysVol::getDefX(const GeoVAlignmentStore* store) con
 
 unsigned int GeoFullPhysVol::getNChildNodes() const 
 {
+  std::scoped_lock<std::mutex> guard(m_mutex);
   return m_daughters.size();
 }
 
 const GeoGraphNode * const * GeoFullPhysVol::getChildNode(unsigned int i) const 
 {
+  std::scoped_lock<std::mutex> guard(m_mutex);
   return &(m_daughters[i]);
 }
 const GeoGraphNode * const * GeoFullPhysVol::findChildNode(const GeoGraphNode * n) const 
 {
+  std::scoped_lock<std::mutex> guard(m_mutex);
   std::vector<const GeoGraphNode *>::const_iterator i = std::find(m_daughters.begin(),m_daughters.end(),n);
   if (i==m_daughters.end()) {
     return nullptr;

GeoModelCore/GeoModelKernel/src/GeoPhysVol.cxx

@@ -19,13 +19,13 @@ GeoPhysVol::GeoPhysVol(const GeoLogVol* LogVol)
 
 GeoPhysVol::~GeoPhysVol()
 {
-  std::lock_guard<std::mutex> lk(m_muxVec);
+  std::scoped_lock<std::mutex> lk(m_muxVec);
   for(const GeoGraphNode* daughter : m_daughters) daughter->unref();
 }
 
 void GeoPhysVol::add(GeoGraphNode* graphNode)
 {
-  std::lock_guard<std::mutex> lk(m_muxVec);
+  std::scoped_lock<std::mutex> lk(m_muxVec);
   m_daughters.push_back(graphNode);
   graphNode->ref();
   graphNode->dockTo(this);
@@ -88,7 +88,7 @@ void GeoPhysVol::exec(GeoNodeAction *action) const
   }
   else {
     // FIXME: m_daughters access is now protected in other methods, but having the lock here makes a deadlock
-    // std::lock_guard<std::mutex> lk(m_muxVec);
+    // std::scoped_lock<std::mutex> lk(m_muxVec);
     // TODO: Think more thouroughly about thread-safe of this class...!!
     for(size_t c = 0; c < m_daughters.size (); c++) {
       m_daughters[c]->exec(action);
@@ -195,19 +195,19 @@ GeoTrf::Transform3D GeoPhysVol::getDefX(const GeoVAlignmentStore* store) const {
 
 unsigned int GeoPhysVol::getNChildNodes() const 
 {
-  std::lock_guard<std::mutex> lk(m_muxVec);
+  std::scoped_lock<std::mutex> lk(m_muxVec);
   return m_daughters.size();
 }
 
 const GeoGraphNode * const * GeoPhysVol::getChildNode(unsigned int i) const 
 {
-  std::lock_guard<std::mutex> lk(m_muxVec);
+  std::scoped_lock<std::mutex> lk(m_muxVec);
   return &(m_daughters[i]);
 }
 
 const GeoGraphNode * const * GeoPhysVol::findChildNode(const GeoGraphNode * n) const 
 {
-  std::lock_guard<std::mutex> lk(m_muxVec);
+  std::scoped_lock<std::mutex> lk(m_muxVec);
   std::vector<const GeoGraphNode *>::const_iterator i = std::find(m_daughters.begin(),m_daughters.end(),n);
   if (i==m_daughters.end()) {
     return nullptr;

GeoModelCore/GeoModelKernel/src/GeoVFullPhysVol.cxx

@@ -30,6 +30,8 @@ const GeoTrf::Transform3D & GeoVFullPhysVol::getAbsoluteTransform(GeoVAlignmentS
   //------------------------------------------------------------------------------------------------//     
   if(isShared()) throw std::runtime_error(errorMessage);
 
+  std::scoped_lock<std::mutex> guard(m_mutex);
+  
   if(store==nullptr && !m_absPosInfo) m_absPosInfo = new GeoAbsPositionInfo();
 
   //
@@ -78,6 +80,7 @@ const GeoTrf::Transform3D & GeoVFullPhysVol::getAbsoluteTransform(GeoVAlignmentS
 const GeoTrf::Transform3D& GeoVFullPhysVol::getCachedAbsoluteTransform(const GeoVAlignmentStore* store) const
 {
   if(store==nullptr) {
+    std::scoped_lock<std::mutex> guard(m_mutex);
     if(m_absPosInfo->getAbsTransform()) return *m_absPosInfo->getAbsTransform();
   }
   else {
@@ -90,6 +93,7 @@ const GeoTrf::Transform3D& GeoVFullPhysVol::getCachedAbsoluteTransform(const Geo
 
 void GeoVFullPhysVol::clearPositionInfo() const
 {
+  std::scoped_lock<std::mutex> guard(m_mutex);
   delete m_absPosInfo;
   m_absPosInfo = nullptr;
 }
@@ -105,6 +109,8 @@ const GeoTrf::Transform3D& GeoVFullPhysVol::getDefAbsoluteTransform(GeoVAlignmen
   //------------------------------------------------------------------------------------------------//     
   if(isShared()) throw std::runtime_error(errorMessage);
 
+  std::scoped_lock<std::mutex> guard(m_mutex);
+  
   if(store==nullptr && !m_absPosInfo) m_absPosInfo = new GeoAbsPositionInfo();
 
   //
@@ -153,6 +159,7 @@ const GeoTrf::Transform3D& GeoVFullPhysVol::getDefAbsoluteTransform(GeoVAlignmen
 const GeoTrf::Transform3D& GeoVFullPhysVol::getCachedDefAbsoluteTransform(const GeoVAlignmentStore* store) const
 {
   if(store==nullptr) {
+    std::scoped_lock<std::mutex> guard(m_mutex);
     if(m_absPosInfo->getDefAbsTransform()) return *m_absPosInfo->getDefAbsTransform();
   }
   else {