From 3a3be6d66a1cd0a9489b69d36e93ea05713f1f81 Mon Sep 17 00:00:00 2001
From: Susumu Oda <susumu.oda@cern.ch>
Date: Fri, 1 Mar 2019 15:02:28 +0000
Subject: [PATCH] Revert SCT_AlignCondAlg to AthAlgorithm. Lock recursive mutex
 at the begining of each method. (ATLASRECTS-4824, ATLASSIM-3931)

---
 .../src/SCT_AlignCondAlg.cxx                  | 14 +++++-----
 .../src/SCT_AlignCondAlg.h                    | 21 ++++++++++++---
 .../InDetReadoutGeometry/SiDetectorElement.h  | 11 ++++----
 .../src/SiDetectorElement.cxx                 | 27 +++++++++----------
 4 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/InnerDetector/InDetConditions/SCT_ConditionsAlgorithms/src/SCT_AlignCondAlg.cxx b/InnerDetector/InDetConditions/SCT_ConditionsAlgorithms/src/SCT_AlignCondAlg.cxx
index fd9581a0323c..bd970519288d 100644
--- a/InnerDetector/InDetConditions/SCT_ConditionsAlgorithms/src/SCT_AlignCondAlg.cxx
+++ b/InnerDetector/InDetConditions/SCT_ConditionsAlgorithms/src/SCT_AlignCondAlg.cxx
@@ -10,7 +10,7 @@
 #include <memory>
 
 SCT_AlignCondAlg::SCT_AlignCondAlg(const std::string& name, ISvcLocator* pSvcLocator)
-  : ::AthReentrantAlgorithm(name, pSvcLocator)
+  : ::AthAlgorithm(name, pSvcLocator)
   , m_writeKey{"SCTAlignmentStore", "SCTAlignmentStore"}
   , m_condSvc{"CondSvc", name}
   , m_detManager{nullptr}
@@ -44,12 +44,12 @@ StatusCode SCT_AlignCondAlg::initialize()
   return StatusCode::SUCCESS;
 }
 
-StatusCode SCT_AlignCondAlg::execute(const EventContext& ctx) const
+StatusCode SCT_AlignCondAlg::execute()
 {
   ATH_MSG_DEBUG("execute " << name());
 
   // ____________ Construct Write Cond Handle and check its validity ____________
-  SG::WriteCondHandle<GeoAlignmentStore> writeHandle{m_writeKey, ctx};
+  SG::WriteCondHandle<GeoAlignmentStore> writeHandle{m_writeKey};
 
   // Do we have a valid Write Cond Handle for current time?
   if (writeHandle.isValid()) {
@@ -72,7 +72,7 @@ StatusCode SCT_AlignCondAlg::execute(const EventContext& ctx) const
 
   if (not m_useDynamicAlignFolders.value()) { // Static
     // ____________ Get Read Cond Object ____________
-    SG::ReadCondHandle<AlignableTransformContainer> readHandleStatic{m_readKeyStatic, ctx};
+    SG::ReadCondHandle<AlignableTransformContainer> readHandleStatic{m_readKeyStatic};
     const AlignableTransformContainer* readCdoStatic{*readHandleStatic};
     if (readCdoStatic==nullptr) {
       ATH_MSG_FATAL("Null pointer to the read conditions object of " << m_readKeyStatic.key());
@@ -91,19 +91,19 @@ StatusCode SCT_AlignCondAlg::execute(const EventContext& ctx) const
     }
   } else { // Dynamic
     // ____________ Get Read Cond Object ____________
-    SG::ReadCondHandle<CondAttrListCollection> readHandleDynamicL1{m_readKeyDynamicL1, ctx};
+    SG::ReadCondHandle<CondAttrListCollection> readHandleDynamicL1{m_readKeyDynamicL1};
     const CondAttrListCollection* readCdoDynamicL1{*readHandleDynamicL1};
     if (readCdoDynamicL1==nullptr) {
       ATH_MSG_FATAL("Null pointer to the read conditions object of " << m_readKeyDynamicL1.key());
       return StatusCode::FAILURE;
     }
-    SG::ReadCondHandle<CondAttrListCollection> readHandleDynamicL2{m_readKeyDynamicL2, ctx};
+    SG::ReadCondHandle<CondAttrListCollection> readHandleDynamicL2{m_readKeyDynamicL2};
     const CondAttrListCollection* readCdoDynamicL2{*readHandleDynamicL2};
     if (readCdoDynamicL2==nullptr) {
       ATH_MSG_FATAL("Null pointer to the read conditions object of " << readHandleDynamicL2.key());
       return StatusCode::FAILURE;
     }
-    SG::ReadCondHandle<AlignableTransformContainer> readHandleDynamicL3{m_readKeyDynamicL3, ctx};
+    SG::ReadCondHandle<AlignableTransformContainer> readHandleDynamicL3{m_readKeyDynamicL3};
     const AlignableTransformContainer* readCdoDynamicL3{*readHandleDynamicL3};
     if (readCdoDynamicL3==nullptr) {
       ATH_MSG_FATAL("Null pointer to the read conditions object of " << readHandleDynamicL3.key());
diff --git a/InnerDetector/InDetConditions/SCT_ConditionsAlgorithms/src/SCT_AlignCondAlg.h b/InnerDetector/InDetConditions/SCT_ConditionsAlgorithms/src/SCT_AlignCondAlg.h
index daab6ebb54b6..8db0a9064031 100644
--- a/InnerDetector/InDetConditions/SCT_ConditionsAlgorithms/src/SCT_AlignCondAlg.h
+++ b/InnerDetector/InDetConditions/SCT_ConditionsAlgorithms/src/SCT_AlignCondAlg.h
@@ -5,7 +5,7 @@
 #ifndef SCT_CONDITIONSALGORITHMS_SCT_ALIGNCONDALG_H
 #define SCT_CONDITIONSALGORITHMS_SCT_ALIGNCONDALG_H
 
-#include "AthenaBaseComps/AthReentrantAlgorithm.h"
+#include "AthenaBaseComps/AthAlgorithm.h"
 #include "StoreGate/ReadCondHandleKey.h"
 #include "StoreGate/WriteCondHandleKey.h"
 
@@ -19,14 +19,29 @@ namespace InDetDD {
   class SCT_DetectorManager;
 }
 
-class SCT_AlignCondAlg : public AthReentrantAlgorithm
+// SCT_AlignCondAlg cannot inherit AthReentrantAlgorithm.
+// SCT_AlignCondAlg::execute uses the following methods.
+//  InDetDD::InDetDetectorManager::align
+//   InDetDD::InDetDetectorManager::processAlignmentContainer
+//    InDetDD::InDetDetectorManager::processKey
+//     InDetDD::SCT_DetectorManager::setAlignableTransformDelta
+//      InDetDD::SiDetectorElement::defModuleTransform
+//       InDetDD::SiDetectorElement::defTransform
+//        InDetDD::SiDetectorElement::defTransformCLHEP
+//         GeoVFullPhysVol::getDefAbsoluteTransform
+// GeoVFullPhysVol::getDefAbsoluteTransform is used without argument.
+// To be thread-safe, we need to pass non-const GeoVAlignmentStore pointer.
+// However, we cannot give non-const pointer for SiDetectorElement
+// in SCT_DetectorManager in the above chain.
+
+class SCT_AlignCondAlg : public AthAlgorithm
 {
  public:
   SCT_AlignCondAlg(const std::string& name, ISvcLocator* pSvcLocator);
   virtual ~SCT_AlignCondAlg() override = default;
 
   virtual StatusCode initialize() override;
-  virtual StatusCode execute(const EventContext& ctx) const override;
+  virtual StatusCode execute() override;
   virtual StatusCode finalize() override;
 
  private:
diff --git a/InnerDetector/InDetDetDescr/InDetReadoutGeometry/InDetReadoutGeometry/SiDetectorElement.h b/InnerDetector/InDetDetDescr/InDetReadoutGeometry/InDetReadoutGeometry/SiDetectorElement.h
index 8a0f00554497..6400ac183d1c 100644
--- a/InnerDetector/InDetDetDescr/InDetReadoutGeometry/InDetReadoutGeometry/SiDetectorElement.h
+++ b/InnerDetector/InDetDetDescr/InDetReadoutGeometry/InDetReadoutGeometry/SiDetectorElement.h
@@ -666,8 +666,8 @@ namespace InDetDD {
     
     inline Amg::Vector3D SiDetectorElement::globalPosition(const Amg::Vector2D &localPos) const
     {
-      if (!m_cacheValid) updateCache();
       std::lock_guard<std::recursive_mutex> lock(m_mutex);
+      if (!m_cacheValid) updateCache();
       return m_center + localPos[Trk::distEta] * m_etaAxis + localPos[Trk::distPhi] * m_phiAxis;
     }
 
@@ -678,8 +678,8 @@ namespace InDetDD {
     
      inline HepGeom::Point3D<double> SiDetectorElement::globalPositionCLHEP(const Amg::Vector2D &localPos) const
     {
-      if (!m_cacheValid) updateCache();
       std::lock_guard<std::recursive_mutex> lock(m_mutex);
+      if (!m_cacheValid) updateCache();
       return m_centerCLHEP + localPos[Trk::distEta] * m_etaAxisCLHEP + localPos[Trk::distPhi] * m_phiAxisCLHEP;
     }
      //here
@@ -695,15 +695,16 @@ namespace InDetDD {
     
     inline Amg::Vector2D SiDetectorElement::localPosition(const HepGeom::Point3D<double> & globalPosition) const
     {
-      if (!m_cacheValid) updateCache();
       std::lock_guard<std::recursive_mutex> lock(m_mutex);
+      if (!m_cacheValid) updateCache();
       HepGeom::Vector3D<double> relativePos = globalPosition - m_centerCLHEP;
       return Amg::Vector2D(relativePos.dot(m_phiAxisCLHEP), relativePos.dot(m_etaAxisCLHEP));
     }
 
-    inline Amg::Vector2D SiDetectorElement::localPosition(const Amg::Vector3D & globalPosition) const{
-      if (!m_cacheValid) updateCache();
+    inline Amg::Vector2D SiDetectorElement::localPosition(const Amg::Vector3D & globalPosition) const
+    {
       std::lock_guard<std::recursive_mutex> lock(m_mutex);
+      if (!m_cacheValid) updateCache();
       Amg::Vector3D relativePos = globalPosition - m_center;
       return Amg::Vector2D(relativePos.dot(m_phiAxis), relativePos.dot(m_etaAxis));
     }
diff --git a/InnerDetector/InDetDetDescr/InDetReadoutGeometry/src/SiDetectorElement.cxx b/InnerDetector/InDetDetDescr/InDetReadoutGeometry/src/SiDetectorElement.cxx
index 1369b8dae36f..bd2a80a8c965 100644
--- a/InnerDetector/InDetDetDescr/InDetReadoutGeometry/src/SiDetectorElement.cxx
+++ b/InnerDetector/InDetDetDescr/InDetReadoutGeometry/src/SiDetectorElement.cxx
@@ -381,19 +381,17 @@ SiDetectorElement::transformHit() const
 const Amg::Transform3D &
 SiDetectorElement::transform() const
 {
-  if (!m_cacheValid) updateCache();
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
+  if (!m_cacheValid) updateCache();
   return m_transform;
 }
 
 const HepGeom::Transform3D &
 SiDetectorElement::transformCLHEP() const
 {
-  //if (!m_cacheValid) updateCache();
-  //return m_transform;
   //stuff to get the CLHEP version of the local to global transform.
-  if (!m_cacheValid) updateCache();
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
+  if (!m_cacheValid) updateCache();
   return m_transformCLHEP;
 }
 
@@ -507,48 +505,48 @@ SiDetectorElement::isModuleFrame() const
 const Amg::Vector3D & 
 SiDetectorElement::center() const
 {
-  if (!m_cacheValid) updateCache();
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
+  if (!m_cacheValid) updateCache();
   return m_center;
 }
 
 const Amg::Vector3D & 
 SiDetectorElement::normal() const
 {
-  if (!m_cacheValid) updateCache();
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
+  if (!m_cacheValid) updateCache();
   return m_normal;
 }
 
 const HepGeom::Vector3D<double> & 
 SiDetectorElement::etaAxisCLHEP() const
 {
-  if (!m_cacheValid) updateCache();
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
+  if (!m_cacheValid) updateCache();
   return m_etaAxisCLHEP;
 }
 
 const HepGeom::Vector3D<double> & 
 SiDetectorElement::phiAxisCLHEP() const
 {
-  if (!m_cacheValid) updateCache();
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
+  if (!m_cacheValid) updateCache();
   return m_phiAxisCLHEP;
 }
 
 const Amg::Vector3D & 
 SiDetectorElement::etaAxis() const
 {
-  if (!m_cacheValid) updateCache();
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
+  if (!m_cacheValid) updateCache();
   return m_etaAxis;
 }
 
 const Amg::Vector3D & 
 SiDetectorElement::phiAxis() const
 {
-  if (!m_cacheValid) updateCache();
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
+  if (!m_cacheValid) updateCache();
   return m_phiAxis;
 }
 
@@ -629,6 +627,7 @@ bool SiDetectorElement::isNextToInnermostPixelLayer() const
 // compute sin(tilt angle) at center:
 double SiDetectorElement::sinTilt() const
 {
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (!m_cacheValid) updateCache();
 
   // Tilt is defined as the angle between a refVector and the sensor normal.
@@ -643,7 +642,6 @@ double SiDetectorElement::sinTilt() const
   // HepGeom::Vector3D<double> refVector(m_center.x(), m_center.y(), 0);
   // return (refVector.cross(m_normal)).z()/refVector.mag();
   // or the equivalent
-  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   return (m_center.x() * m_normal.y() - m_center.y() * m_normal.x()) / m_center.perp();
 }
 
@@ -660,6 +658,7 @@ double SiDetectorElement::sinTilt() const
 
 double SiDetectorElement::sinTilt(const HepGeom::Point3D<double> &globalPos) const
 { 
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (!m_cacheValid) updateCache();
 
   // It is assumed that the global position is already in the plane of the element.
@@ -667,7 +666,6 @@ double SiDetectorElement::sinTilt(const HepGeom::Point3D<double> &globalPos) con
   // tilt angle is not defined for the endcap
   if (isEndcap()) return 0;
   
-  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   // Angle between normal and radial vector.
   //HepGeom::Vector3D<double> refVector(globalPos.x(), globalPos.y(), 0);
   //return (refVector.cross(m_normal)).z()/refVector.mag();
@@ -677,7 +675,7 @@ double SiDetectorElement::sinTilt(const HepGeom::Point3D<double> &globalPos) con
 
 double SiDetectorElement::sinStereo() const
 {
-
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (!m_cacheValid) updateCache();
 
   // Stereo is the angle between a refVector and a vector along the strip/pixel in eta direction.
@@ -702,7 +700,6 @@ double SiDetectorElement::sinStereo() const
   //           = -(center cross etaAxis) . zAxis
   //           = (etaAxis cross center). z() 
 
-  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   double sinStereo;
   if (isBarrel()) {
     sinStereo = m_phiAxis.z();
@@ -726,9 +723,9 @@ double SiDetectorElement::sinStereo(const HepGeom::Point3D<double> &globalPos) c
   // sinStereo =  (refVector cross stripAxis) . normal
   //
 
+  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   if (!m_cacheValid) updateCache();
 
-  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   double sinStereo;
   if (isBarrel()) {
     if (m_design->shape() != InDetDD::Trapezoid) {
-- 
GitLab