[geom] Fix stale thread-local navigators across manager lifetimes

geom/geom/src/TGeoManager.cxx

@@ -236,6 +236,7 @@ in order to enhance rays.
 \image html geom_random2.jpg
 */
 
+#include <atomic>
 #include <cstdlib>
 #include <iostream>
 #include <fstream>
@@ -305,6 +306,33 @@ TGeoManager::EDefaultUnits TGeoManager::fgDefaultUnits = TGeoManager::kRootUnits
 TGeoManager::ThreadsMap_t *TGeoManager::fgThreadId = nullptr;
 static Bool_t gGeometryLocked = kFALSE;
 
+namespace {
+
+struct TGeoManagerThreadState {
+   const TGeoManager *fManager = nullptr;
+   TGeoNavigator *fNavigator = nullptr;
+   ULong64_t fNavigatorGeneration = 0;
+   Int_t fThreadId = -1;
+   ULong64_t fThreadIdGeneration = 0;
+};
+
+TGeoManagerThreadState &GetGeoManagerThreadState()
+{
+   TTHREAD_TLS(TGeoManagerThreadState) state;
+   return state;
+}
+
+// Thread-local navigator pointers and thread ordinals cannot be reset by the thread deleting a manager.
+// Advancing this generation on destructive/global state transitions makes every thread refresh on its next access.
+std::atomic<ULong64_t> gGeoManagerThreadStateGeneration{1};
+
+void InvalidateGeoManagerThreadState()
+{
+   gGeoManagerThreadStateGeneration.fetch_add(1, std::memory_order_release);
+}
+
+} // namespace
+
 ////////////////////////////////////////////////////////////////////////////////
 /// Default constructor.
 
@@ -807,8 +835,13 @@ TGeoNavigator *TGeoManager::AddNavigator()
    TGeoNavigator *nav = array->AddNavigator();
    if (fClosed)
       nav->GetCache()->BuildInfoBranch();
-   if (fMultiThread)
+   if (fMultiThread) {
+      auto &state = GetGeoManagerThreadState();
+      state.fManager = this;
+      state.fNavigator = nav;
+      state.fNavigatorGeneration = gGeoManagerThreadStateGeneration.load(std::memory_order_acquire);
       fgMutex.unlock();
+   }
    return nav;
 }
 
@@ -817,27 +850,37 @@ TGeoNavigator *TGeoManager::AddNavigator()
 
 TGeoNavigator *TGeoManager::GetCurrentNavigator() const
 {
-   TTHREAD_TLS(TGeoNavigator *) tnav = nullptr;
    if (!fMultiThread)
       return fCurrentNavigator;
-   TGeoNavigator *nav = tnav; // TTHREAD_TLS_GET(TGeoNavigator*,tnav);
-   if (nav)
-      return nav;
+   auto &state = GetGeoManagerThreadState();
+   const auto generation = gGeoManagerThreadStateGeneration.load(std::memory_order_acquire);
+   if (state.fNavigator && state.fManager == this && state.fNavigatorGeneration == generation)
+      return state.fNavigator;
+
+   std::lock_guard<std::mutex> lock(fgMutex);
    std::thread::id threadId = std::this_thread::get_id();
    NavigatorsMap_t::const_iterator it = fNavigators.find(threadId);
-   if (it == fNavigators.end())
+   if (it == fNavigators.end()) {
+      state.fManager = this;
+      state.fNavigator = nullptr;
+      state.fNavigatorGeneration = generation;
       return nullptr;
+   }
    TGeoNavigatorArray *array = it->second;
-   nav = array->GetCurrentNavigator();
-   tnav = nav; // TTHREAD_TLS_SET(TGeoNavigator*,tnav,nav);
-   return nav;
+   state.fManager = this;
+   state.fNavigator = array->GetCurrentNavigator();
+   state.fNavigatorGeneration = generation;
+   return state.fNavigator;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 /// Get list of navigators for the calling thread.
 
 TGeoNavigatorArray *TGeoManager::GetListOfNavigators() const
 {
+   std::unique_lock<std::mutex> lock(fgMutex, std::defer_lock);
+   if (fMultiThread)
+      lock.lock();
    std::thread::id threadId = std::this_thread::get_id();
    NavigatorsMap_t::const_iterator it = fNavigators.find(threadId);
    if (it == fNavigators.end())
@@ -851,6 +894,9 @@ TGeoNavigatorArray *TGeoManager::GetListOfNavigators() const
 
 Bool_t TGeoManager::SetCurrentNavigator(Int_t index)
 {
+   std::unique_lock<std::mutex> lock(fgMutex, std::defer_lock);
+   if (fMultiThread)
+      lock.lock();
    std::thread::id threadId = std::this_thread::get_id();
    NavigatorsMap_t::const_iterator it = fNavigators.find(threadId);
    if (it == fNavigators.end()) {
@@ -865,8 +911,14 @@ Bool_t TGeoManager::SetCurrentNavigator(Int_t index)
       std::cout << "  thread id: " << threadId << std::endl;
       return kFALSE;
    }
-   if (!fMultiThread)
+   if (fMultiThread) {
+      auto &state = GetGeoManagerThreadState();
+      state.fManager = this;
+      state.fNavigator = nav;
+      state.fNavigatorGeneration = gGeoManagerThreadStateGeneration.load(std::memory_order_acquire);
+   } else {
       fCurrentNavigator = nav;
+   }
    return kTRUE;
 }
 
@@ -883,6 +935,7 @@ void TGeoManager::SetNavigatorsLock(Bool_t flag)
 
 void TGeoManager::ClearNavigators()
 {
+   InvalidateGeoManagerThreadState();
    if (fMultiThread)
       fgMutex.lock();
    TGeoNavigatorArray *arr = nullptr;
@@ -907,6 +960,7 @@ void TGeoManager::RemoveNavigator(const TGeoNavigator *nav)
       TGeoNavigatorArray *arr = (*it).second;
       if (arr) {
          if ((TGeoNavigator *)arr->Remove((TObject *)nav)) {
+            InvalidateGeoManagerThreadState();
             delete nav;
             if (!arr->GetEntries())
                fNavigators.erase(it);
@@ -944,6 +998,7 @@ void TGeoManager::SetMaxThreads(Int_t nthreads)
       ClearThreadsMap();
       ClearThreadData();
    }
+   InvalidateGeoManagerThreadState();
    fMaxThreads = nthreads + 1;
    if (fMaxThreads > 0) {
       fMultiThread = kTRUE;
@@ -986,6 +1041,7 @@ void TGeoManager::CreateThreadData() const
 
 void TGeoManager::ClearThreadsMap()
 {
+   InvalidateGeoManagerThreadState();
    if (gGeoManager && !gGeoManager->IsMultiThread())
       return;
    fgMutex.lock();
@@ -1001,23 +1057,27 @@ void TGeoManager::ClearThreadsMap()
 
 Int_t TGeoManager::ThreadId()
 {
-   TTHREAD_TLS(Int_t) tid = -1;
-   Int_t ttid = tid; // TTHREAD_TLS_GET(Int_t,tid);
-   if (ttid > -1)
-      return ttid;
-   if (gGeoManager && !gGeoManager->IsMultiThread())
+   auto &state = GetGeoManagerThreadState();
+   const auto generation = gGeoManagerThreadStateGeneration.load(std::memory_order_acquire);
+   if (state.fThreadId > -1 && state.fThreadIdGeneration == generation)
+      return state.fThreadId;
+   if (gGeoManager && !gGeoManager->IsMultiThread()) {
+      state.fThreadId = 0;
+      state.fThreadIdGeneration = generation;
       return 0;
+   }
    std::thread::id threadId = std::this_thread::get_id();
+   std::lock_guard<std::mutex> lock(fgMutex);
    TGeoManager::ThreadsMapIt_t it = fgThreadId->find(threadId);
-   if (it != fgThreadId->end())
-      return it->second;
-   // Map needs to be updated.
-   fgMutex.lock();
+   if (it != fgThreadId->end()) {
+      state.fThreadId = it->second;
+      state.fThreadIdGeneration = generation;
+      return state.fThreadId;
+   }
    (*fgThreadId)[threadId] = fgNumThreads;
-   tid = fgNumThreads; // TTHREAD_TLS_SET(Int_t,tid,fgNumThreads);
-   ttid = fgNumThreads++;
-   fgMutex.unlock();
-   return ttid;
+   state.fThreadId = fgNumThreads++;
+   state.fThreadIdGeneration = generation;
+   return state.fThreadId;
 }
 
 ////////////////////////////////////////////////////////////////////////////////

geom/geom/src/TGeoNode.cxx

@@ -323,13 +323,9 @@ void TGeoNode::CheckOverlaps(Double_t ovlp, Option_t *option)
    timer.Start();
    pool.Foreach(
       [&](const std::pair<size_t, size_t> &range) {
-         // one-time init per OS thread
-         static thread_local bool navInit = false;
-         if (!navInit) {
-            if (!geom->GetCurrentNavigator())
-               geom->AddNavigator();
-            navInit = true;
-         }
+         // Make sure this manager has a navigator for the current worker.
+         if (!geom->GetCurrentNavigator())
+            geom->AddNavigator();
 
          std::vector<TGeoOverlapResult> local;
          local.reserve(32);

geom/geom/src/TGeoVolume.cxx

@@ -680,13 +680,9 @@ void TGeoVolume::CheckOverlaps(Double_t ovlp, Option_t *option)
    timer.Start();
    pool.Foreach(
       [&](const std::pair<size_t, size_t> &range) {
-         // one-time init per OS thread
-         static thread_local bool navInit = false;
-         if (!navInit) {
-            if (!geom->GetCurrentNavigator())
-               geom->AddNavigator();
-            navInit = true;
-         }
+         // Make sure this manager has a navigator for the current worker.
+         if (!geom->GetCurrentNavigator())
+            geom->AddNavigator();
 
          std::vector<TGeoOverlapResult> local;
          local.reserve(32);

geom/test/CMakeLists.txt

@@ -20,3 +20,9 @@ endif()
 ROOT_ADD_GTEST(tessellated
   test_tessellated.cxx
   LIBRARIES Geom)
+
+if(imt)
+  ROOT_ADD_GTEST(manager_lifetime
+    test_manager_lifetime.cxx
+    LIBRARIES Geom GeomChecker Imt)
+endif()

geom/test/test_manager_lifetime.cxx

@@ -0,0 +1,123 @@
+#include <atomic>
+#include <thread>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+#include <ROOT/TestSupport.hxx>
+#include <TGeoManager.h>
+#include <TGeoMaterial.h>
+#include <TGeoMedium.h>
+#include <TGeoNavigator.h>
+#include <TGeoVolume.h>
+#include <TROOT.h>
+
+namespace {
+
+TGeoManager *MakeGeometry(int iteration)
+{
+   TGeoManager *geom = nullptr;
+   // TGeoManager::Init deletes the existing gGeoManager, as happens when the
+   // interpreted macro is executed again from the main thread.
+   if (gGeoManager) {
+      ROOT_EXPECT_WARNING_PARTIAL(geom =
+                                     new TGeoManager(TString::Format("world_%d", iteration), "navigator lifetime test"),
+                                  "TGeoManager::Init", "Deleting previous geometry:");
+   } else {
+      geom = new TGeoManager(TString::Format("world_%d", iteration), "navigator lifetime test");
+   }
+   auto *material = new TGeoMaterial("Vacuum", 0., 0., 0.);
+   auto *medium = new TGeoMedium("Vacuum", 1, material);
+   auto *top = geom->MakeBox("top", medium, 10., 10., 10.);
+   auto *sphere = geom->MakeSphere("sphere", medium, 0., 5.);
+   top->AddNode(sphere, 1);
+   geom->SetTopVolume(top);
+   geom->CloseGeometry();
+   return geom;
+}
+
+class ImplicitMTGuard {
+public:
+   explicit ImplicitMTGuard(unsigned int numThreads) { ROOT::EnableImplicitMT(numThreads); }
+   ~ImplicitMTGuard() { ROOT::DisableImplicitMT(); }
+};
+
+} // namespace
+
+TEST(TGeoManager, ConcurrentNavigatorsStayThreadLocal)
+{
+   constexpr unsigned int numThreads = 4;
+   constexpr int numNavigators = 16;
+
+   auto *geom = MakeGeometry(0);
+   geom->SetMaxThreads(numThreads);
+
+   // Start all workers together so navigator creation and lookup overlap.
+   std::atomic<unsigned int> numReady{0};
+   std::atomic<bool> start{false};
+   std::atomic<bool> failed{false};
+   std::vector<std::thread> workers;
+   workers.reserve(numThreads);
+
+   for (unsigned int i = 0; i < numThreads; ++i) {
+      workers.emplace_back([&] {
+         numReady.fetch_add(1, std::memory_order_release);
+         while (!start.load(std::memory_order_acquire))
+            std::this_thread::yield();
+
+         // Every worker owns an independent navigator array. Adding a navigator
+         // here must update only this worker's current-navigator cache, even as
+         // the other workers add their navigators concurrently.
+         TGeoNavigator *first = nullptr;
+         TGeoNavigator *current = nullptr;
+         for (int j = 0; j < numNavigators; ++j) {
+            current = geom->AddNavigator();
+            if (!first)
+               first = current;
+            for (int check = 0; check < 32; ++check) {
+               if (geom->GetCurrentNavigator() != current)
+                  failed.store(true, std::memory_order_relaxed);
+            }
+         }
+
+         // Selecting another navigator must likewise affect only this worker.
+         if (!geom->SetCurrentNavigator(0) || geom->GetCurrentNavigator() != first)
+            failed.store(true, std::memory_order_relaxed);
+      });
+   }
+
+   while (numReady.load(std::memory_order_acquire) != numThreads)
+      std::this_thread::yield();
+   start.store(true, std::memory_order_release);
+
+   for (auto &worker : workers)
+      worker.join();
+
+   EXPECT_FALSE(failed.load(std::memory_order_relaxed));
+   delete gGeoManager;
+}
+
+TEST(TGeoManager, RecreateAfterParallelOverlapCheck)
+{
+   ImplicitMTGuard imtGuard(2);
+
+   // CheckOverlaps populates navigator caches in persistent IMT worker threads.
+   // On the next iteration, MakeGeometry deletes the old manager on the main
+   // thread and creates a new one, while the worker threads remain alive.
+   for (int iteration = 0; iteration < 10; ++iteration) {
+      auto *geom = MakeGeometry(iteration);
+
+      // Foreach waits for all workers before returning. Reused workers must
+      // reject a navigator cached for the previous manager generation and book
+      // a navigator belonging to this manager.
+      geom->CheckOverlaps(0.001);
+
+      // The calling thread must also resolve the current manager's navigator.
+      ASSERT_TRUE(geom->IsMultiThread());
+      ASSERT_NE(geom->GetListOfNavigators(), nullptr);
+      ASSERT_EQ(geom->GetCurrentNavigator(), geom->GetListOfNavigators()->GetCurrentNavigator());
+      EXPECT_EQ(geom->GetStackLevel(), 0);
+   }
+
+   delete gGeoManager;
+}