Fix initialization and thread-safety of SDF::Version() global (#1522)

Replace a static global variable that used a pre-main constructor with a latch-initialized static local variable and mutex to guard the writes. This does not change API but it does change ABI. Signed-off-by: Jeremy Nimmer <[email protected]>
gazebosim · Jan 6, 2025 · 5d9623e · 5d9623e
1 parent aaadeea
commit 5d9623e
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 10 deletions.
diff --git a/include/sdf/SDFImpl.hh b/include/sdf/SDFImpl.hh
@@ -236,6 +236,8 @@ namespace sdf
     public: const std::string &OriginalVersion() const;
 
     /// \brief Get the version
+    /// The result will be set to SDF_VERSION by default, unless overridden by a
+    /// call to the Version(const std::string &) function at runtime.
     /// \return The version as a string
     public: static std::string Version();
 
@@ -290,10 +292,6 @@ namespace sdf
     /// \internal
     /// \brief Pointer to private data.
     private: std::unique_ptr<SDFPrivate> dataPtr;
-
-    /// \brief The SDF version. Set to SDF_VERSION by default, or through
-    /// the Version function at runtime.
-    private: static std::string version;
   };
   /// \}
   }

diff --git a/src/SDF.cc b/src/SDF.cc
@@ -21,9 +21,12 @@
 #include <functional>
 #include <list>
 #include <map>
+#include <mutex>
 #include <string>
 #include <vector>
 
+#include <gz/utils/NeverDestroyed.hh>
+
 #include "sdf/parser.hh"
 #include "sdf/Assert.hh"
 #include "sdf/Console.hh"
@@ -42,10 +45,6 @@ namespace sdf
 {
 inline namespace SDF_VERSION_NAMESPACE
 {
-// TODO(azeey) This violates the Google style guide. Change to a function that
-// returns the version string when possible.
-std::string SDF::version = SDF_VERSION;  // NOLINT(runtime/string)
-
 std::string sdfSharePath()
 {
   std::string sharePath = sdf::getSharePath();
@@ -488,16 +487,33 @@ const std::string &SDF::OriginalVersion() const
   return this->dataPtr->originalVersion;
 }
 
+/////////////////////////////////////////////////
+namespace {
+// Infrastructe to support the SDF::Version(...) static getter and setter.
+struct GlobalVersion {
+  std::mutex mutex;
+  std::string version{SDF_VERSION};
+};
+GlobalVersion& GetMutableGlobalVersionSingleton() {
+  static gz::utils::NeverDestroyed<GlobalVersion> singleton;
+  return singleton.Access();
+}
+}  // namespace
+
 /////////////////////////////////////////////////
 std::string SDF::Version()
 {
-  return version;
+  GlobalVersion& singleton = GetMutableGlobalVersionSingleton();
+  std::lock_guard guard{singleton.mutex};
+  return std::string{singleton.version};
 }
 
 /////////////////////////////////////////////////
 void SDF::Version(const std::string &_version)
 {
-  version = _version;
+  GlobalVersion& singleton = GetMutableGlobalVersionSingleton();
+  std::lock_guard guard{singleton.mutex};
+  singleton.version = _version;
 }
 
 /////////////////////////////////////////////////