Fix potential data race in ESProductResolver

cms-sw · Oct 9, 2024 · 123b6a1 · 123b6a1
1 parent 45e883d
commit 123b6a1
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 15 deletions.
diff --git a/FWCore/Framework/interface/ESProductResolver.h b/FWCore/Framework/interface/ESProductResolver.h
@@ -45,7 +45,7 @@ namespace edm {
       virtual ~ESProductResolver();
 
       // ---------- const member functions ---------------------
-      bool cacheIsValid() const { return cacheIsValid_.load(std::memory_order_acquire); }
+      bool cacheIsValid() const;
 
       void prefetchAsync(WaitingTaskHolder,
                          EventSetupRecordImpl const&,
@@ -106,8 +106,7 @@ namespace edm {
     private:
       // ---------- member data --------------------------------
       ComponentDescription const* description_;
-      CMS_THREAD_SAFE mutable void const* cache_;  //protected by a global mutex
-      mutable std::atomic<bool> cacheIsValid_;
+      mutable std::atomic<void const*> cache_;
 
       // While implementing the set of code changes that enabled support
       // for concurrent IOVs, I have gone to some effort to maintain

diff --git a/FWCore/Framework/src/ESProductResolver.cc b/FWCore/Framework/src/ESProductResolver.cc
@@ -16,6 +16,11 @@
 #include "FWCore/Framework/interface/EventSetupRecordImpl.h"
 #include "FWCore/Utilities/interface/Likely.h"
 
+namespace {
+  constexpr int kInvalidLocation = 0;
+  void const* const kInvalid = &kInvalidLocation;
+}  // namespace
+
 namespace edm {
   namespace eventsetup {
 
@@ -25,17 +30,15 @@ namespace edm {
     }
 
     ESProductResolver::ESProductResolver()
-        : description_(dummyDescription()),
-          cache_(nullptr),
-          cacheIsValid_(false),
-          nonTransientAccessRequested_(false) {}
+        : description_(dummyDescription()), cache_(kInvalid), nonTransientAccessRequested_(false) {}
 
     ESProductResolver::~ESProductResolver() {}
 
+    bool ESProductResolver::cacheIsValid() const { return cache_.load() != kInvalid; }
+
     void ESProductResolver::clearCacheIsValid() {
       nonTransientAccessRequested_.store(false, std::memory_order_release);
-      cache_ = nullptr;
-      cacheIsValid_.store(false, std::memory_order_release);
+      cache_.store(kInvalid);
     }
 
     void ESProductResolver::resetIfTransient() {
@@ -74,15 +77,20 @@ namespace edm {
         nonTransientAccessRequested_.store(true, std::memory_order_release);
       }
 
-      if UNLIKELY (!cacheIsValid()) {
-        cache_ = getAfterPrefetchImpl();
-        cacheIsValid_.store(true, std::memory_order_release);
+      auto cache = cache_.load();
+      if UNLIKELY (cache == kInvalid) {
+        // This safe even if multiple threads get in here simultaneously
+        // because cache_ is atomic and getAfterPrefetchImpl will return
+        // the same pointer on all threads for the same IOV.
+        // This is fast because the vast majority of the time only 1 thread per IOV
+        // will get in here so most of the time only 1 atomic operation.
+        cache_.store(getAfterPrefetchImpl());
+        cache = cache_.load();
       }
-
-      if UNLIKELY (nullptr == cache_) {
+      if UNLIKELY (cache == nullptr) {
         throwMakeException(iRecord, iKey);
       }
-      return cache_;
+      return cache;
     }
 
   }  // namespace eventsetup