Merge topic 'instrumentation-index-lock'

0c2fcaec5d instrumentation: Avoid duplicate entries on concurrent indexing

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !11912

Help/manual/cmake-instrumentation.7.rst

@@ -66,14 +66,20 @@ Indexing
 
 Indexing is the process of collating generated instrumentation data. The
 available hooks to trigger indexing include options such as after every build,
-or every :manual:`ctest <ctest(1)>` invocation, and are configured as part of the `v1 Query Files`_.
-Whenever a hook is triggered, an index file is generated containing a list of
-snippet files newer than the previous indexing. This index file is passed to
-user-defined `Callbacks`_ commands to process the data.
+or every :manual:`ctest <ctest(1)>` invocation, and are configured as part of
+the `v1 Query Files`_. Whenever a hook is triggered, an index file is generated
+containing a list of snippet files newer than the previous indexing. This index
+file is passed to user-defined `Callbacks`_ commands to process the data.
 
 Indexing and can also be performed by manually invoking
 :option:`ctest --collect-instrumentation`.
 
+Indexing, and the subsequent callbacks, will not occur concurrently in a
+single build tree. When multiple hooks trigger indexing at the same time,
+a file-based lock is used to ensure one indexing completes, executes all of its
+callbacks, and deletes the instrumentation data before the next indexing can
+begin.
+
 .. _`cmake-instrumentation Callbacks`:
 
 Callbacks

Source/cmInstrumentation.cxx

@@ -348,6 +348,8 @@ int cmInstrumentation::CollectTimingData(cmInstrumentationQuery::Hook hook)
     return 0;
   }
 
+  this->LockIndexing();
+
   // Touch index file immediately to claim snippets
   std::string suffix_time = ComputeSuffixTime();
   std::string const& index_name = cmStrCat("index-", suffix_time, ".json");
@@ -428,6 +430,8 @@ int cmInstrumentation::CollectTimingData(cmInstrumentationQuery::Hook hook)
   this->RemoveOldFiles("content");
   this->RemoveOldFiles("trace");
 
+  this->indexLock.Release();
+
   return 0;
 }
 
@@ -837,7 +841,7 @@ int cmInstrumentation::SpawnBuildDaemon()
   // preBuild Hook
   if (this->LockBuildDaemon()) {
     // Release lock before spawning the build daemon, to prevent blocking it.
-    this->lock.Release();
+    this->buildLock.Release();
     this->CollectTimingData(cmInstrumentationQuery::Hook::PreBuild);
   }
 
@@ -859,11 +863,26 @@ int cmInstrumentation::SpawnBuildDaemon()
 // Prevent multiple build daemons from running simultaneously
 bool cmInstrumentation::LockBuildDaemon()
 {
-  std::string const lockFile = cmStrCat(this->timingDirv1, "/.build.lock");
-  if (!cmSystemTools::FileExists(lockFile)) {
-    cmSystemTools::Touch(lockFile, true);
+  // 0 = non-blocking, 0s timeout
+  return this->AcquireLock(".build.lock", this->buildLock, 0);
+}
+
+// Prevent multiple index processes from claiming snippets simultaneously
+bool cmInstrumentation::LockIndexing()
+{
+  return this->AcquireLock(".index.lock", this->indexLock,
+                           // -1 = no timeout
+                           static_cast<unsigned long>(-1));
+}
+
+bool cmInstrumentation::AcquireLock(std::string const& lock_file,
+                                    cmFileLock& lock, unsigned long timeout)
+{
+  std::string const lock_path = cmStrCat(this->timingDirv1, '/', lock_file);
+  if (!cmSystemTools::FileExists(lock_path)) {
+    cmSystemTools::Touch(lock_path, true);
   }
-  return this->lock.Lock(lockFile, 0).IsOk();
+  return lock.Lock(lock_path, timeout).IsOk();
 }
 
 /*
@@ -890,6 +909,7 @@ int cmInstrumentation::CollectTimingAfterBuild(int ppid)
   int ret = this->InstrumentCommand(
     "build", {}, [waitForBuild]() { return waitForBuild(); }, cm::nullopt,
     cm::nullopt, LoadQueriesAfter::Yes);
+  this->buildLock.Release();
   this->CollectTimingData(cmInstrumentationQuery::Hook::PostBuild);
   return ret;
 }

Source/cmInstrumentation.h

@@ -70,6 +70,7 @@ public:
   int CollectTimingData(cmInstrumentationQuery::Hook hook);
   int SpawnBuildDaemon();
   bool LockBuildDaemon();
+  bool LockIndexing();
   int CollectTimingAfterBuild(int ppid);
   void AddHook(cmInstrumentationQuery::Hook hook);
   void AddOption(cmInstrumentationQuery::Option option);
@@ -87,6 +88,8 @@ public:
 
 private:
   Json::Value ReadJsonSnippet(std::string const& file_name);
+  bool AcquireLock(std::string const& lock_file, cmFileLock& lock,
+                   unsigned long timeout);
   void WriteInstrumentationJson(Json::Value& index,
                                 std::string const& directory,
                                 std::string const& file_name);
@@ -134,5 +137,6 @@ private:
   cmsys::SystemInformation& GetSystemInformation();
 #endif
   int writtenJsonQueries = 0;
-  cmFileLock lock;
+  cmFileLock buildLock; // non-blocking
+  cmFileLock indexLock; // never held alongside buildLock
 };