From 56f0b6496a9c87e9af8ab67ded397b6bcca6625d Mon Sep 17 00:00:00 2001
From: Matti Kortelainen <matti.kortelainen@cern.ch>
Date: Mon, 5 Jun 2023 12:31:02 +0200
Subject: [PATCH] Fix a race condition in StMeasurementDetSet

The concurrent writes to empty_ and ready_ are not protected
---
 .../MeasurementDet/src/TkMeasurementDetSet.h  | 61 +++++++++++--------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h b/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h
index 6c5f3f89c317a..92a85c6a5603e 100644
--- a/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h
+++ b/RecoTracker/MeasurementDet/src/TkMeasurementDetSet.h
@@ -20,6 +20,9 @@ class Phase2StripCPE;
 #include "CondFormats/SiStripObjects/interface/SiStripBadStrip.h"
 #include "CalibFormats/SiStripObjects/interface/SiStripQuality.h"
 
+#include "FWCore/Utilities/interface/thread_safety_macros.h"
+
+#include <atomic>
 #include <unordered_map>
 
 // #define VISTAT
@@ -142,11 +145,9 @@ class StMeasurementDetSet {
 
   StMeasurementDetSet(const StMeasurementConditionSet& cond)
       : conditionSet_(&cond),
-        empty_(cond.nDet(), true),
         activeThisEvent_(cond.nDet(), true),
         detSet_(cond.nDet()),
         detIndex_(cond.nDet(), -1),
-        ready_(cond.nDet(), true),
         theRawInactiveStripDetIds_(),
         stripDefined_(0),
         stripUpdated_(0),
@@ -157,16 +158,16 @@ class StMeasurementDetSet {
   const StMeasurementConditionSet& conditions() const { return *conditionSet_; }
 
   void update(int i, const StripDetset& detSet) {
-    detSet_[i] = detSet;
-    empty_[i] = false;
+    detSet_[i].detSet_ = detSet;
+    detSet_[i].empty_ = false;
   }
 
   void update(int i, int j) {
     assert(j >= 0);
-    assert(empty_[i]);
-    assert(ready_[i]);
+    assert(detSet_[i].empty_);
+    assert(detSet_[i].ready_);
     detIndex_[i] = j;
-    empty_[i] = false;
+    detSet_[i].empty_ = false;
     incReady();
   }
 
@@ -175,19 +176,21 @@ class StMeasurementDetSet {
   unsigned int id(int i) const { return conditions().id(i); }
   int find(unsigned int jd, int i = 0) const { return conditions().find(jd, i); }
 
-  bool empty(int i) const { return empty_[i]; }
+  bool empty(int i) const { return detSet_[i].empty_; }
   bool isActive(int i) const { return activeThisEvent_[i] && conditions().isActiveThisPeriod(i); }
 
   void setEmpty(int i) {
-    empty_[i] = true;
+    detSet_[i].empty_ = true;
     activeThisEvent_[i] = true;
   }
   void setUpdated(int i) { stripUpdated_[i] = true; }
 
   void setEmpty() {
     printStat();
-    std::fill(empty_.begin(), empty_.end(), true);
-    std::fill(ready_.begin(), ready_.end(), true);
+    for (auto& d : detSet_) {
+      d.empty_ = true;
+      d.ready_ = true;
+    }
     std::fill(detIndex_.begin(), detIndex_.end(), -1);
     std::fill(activeThisEvent_.begin(), activeThisEvent_.end(), true);
     incTot(size());
@@ -198,16 +201,16 @@ class StMeasurementDetSet {
   void setActiveThisEvent(int i, bool active) {
     activeThisEvent_[i] = active;
     if (!active)
-      empty_[i] = true;
+      detSet_[i].empty_ = true;
   }
 
   edm::Handle<edmNew::DetSetVector<SiStripCluster>>& handle() { return handle_; }
   const edm::Handle<edmNew::DetSetVector<SiStripCluster>>& handle() const { return handle_; }
   // StripDetset & detSet(int i) { return detSet_[i]; }
   const StripDetset& detSet(int i) const {
-    if (ready_[i])
-      const_cast<StMeasurementDetSet*>(this)->getDetSet(i);
-    return detSet_[i];
+    if (detSet_[i].ready_)
+      getDetSet(i);
+    return detSet_[i].detSet_;
   }
 
   //// ------- pieces for on-demand unpacking --------
@@ -227,16 +230,18 @@ class StMeasurementDetSet {
   }
 
 private:
-  void getDetSet(int i) {
+  void getDetSet(int i) const {
+    const auto& det = detSet_[i];
     if (detIndex_[i] >= 0) {
-      detSet_[i].set(*handle_, handle_->item(detIndex_[i]));
-      empty_[i] = false;  // better be false already
+      // edmNew::DetSet<T>::set() internally does an atomic update
+      det.detSet_.set(*handle_, handle_->item(detIndex_[i]));
+      det.empty_ = false;  // better be false already
       incAct();
     } else {  // we should not be here
-      detSet_[i] = StripDetset();
-      empty_[i] = true;
+      det.detSet_ = StripDetset();
+      det.empty_ = true;
     }
-    ready_[i] = false;
+    det.ready_ = false;
     incSet();
   }
 
@@ -247,13 +252,21 @@ class StMeasurementDetSet {
   // Globals, per-event
   edm::Handle<edmNew::DetSetVector<SiStripCluster>> handle_;
 
-  std::vector<bool> empty_;
+  // Helper struct to define only the vector elements as mutable and
+  // to have a vector of atomics without an explicit loop over
+  // elements to set their values
+  struct DetSetHelper {
+    mutable std::atomic<bool> empty_ = true;
+    mutable std::atomic<bool> ready_ = true;  // to be cleaned
+    // only thread-safe non-const member functions are called from a const function
+    CMS_THREAD_SAFE mutable StripDetset detSet_;
+  };
+
   std::vector<bool> activeThisEvent_;
 
   // full reco
-  std::vector<StripDetset> detSet_;
+  std::vector<DetSetHelper> detSet_;
   std::vector<int> detIndex_;
-  std::vector<bool> ready_;  // to be cleaned
 
   // note: not aligned to the index
   std::vector<uint32_t> theRawInactiveStripDetIds_;