Fix intemittent MdrunIOTests segfault

In Modular Simulator, we wish to have two copies of
ComputeGlobalsElement<ComputeGlobalsAlgorithm::VelocityVerlet>. To avoid
making two separate objects, a `static thread_local` singleton was
introduced. However, it caused issues when the ModularSimulator was
built repeatedly in the same process, due to stale pointer persisting in
a static variable.

This commit adds a key-value storage to
ModularSimulatorAlgorithmBuilderHelper, and makes ComputeGlobalsElement
use it instead of static variables. This ensures proper lifetime of
cached pointers and can be used by other Elements for similar goals.

Closes #3791

diff --git a/src/gromacs/modularsimulator/computeglobalselement.cpp b/src/gromacs/modularsimulator/computeglobalselement.cpp
index ae4635d19d2405a122f2c83800fd6ba3c3a0f9b8..28062291c6917dceb190a9fdf700d78c4ef46464 100644 (file)
--- a/src/gromacs/modularsimulator/computeglobalselement.cpp
+++ b/src/gromacs/modularsimulator/computeglobalselement.cpp
@@ -384,10 +384,17 @@ ISimulatorElement* ComputeGlobalsElement<ComputeGlobalsAlgorithm::VelocityVerlet
 {
     // We allow this element to be added multiple times to the call list, but we only want one
     // actual element built
-    static thread_local ISimulatorElement* vvComputeGlobalsElement = nullptr;
-    if (!builderHelper->elementIsStored(vvComputeGlobalsElement))
+    static const std::string key("vvComputeGlobalsElement");
+
+    const std::optional<std::any> cachedValue = builderHelper->getStoredValue(key);
+
+    if (cachedValue)
+    {
+        return std::any_cast<ISimulatorElement*>(cachedValue.value());
+    }
+    else
     {
-        vvComputeGlobalsElement = builderHelper->storeElement(
+        ISimulatorElement* vvComputeGlobalsElement = builderHelper->storeElement(
                 std::make_unique<ComputeGlobalsElement<ComputeGlobalsAlgorithm::VelocityVerlet>>(
                         statePropagatorData, energyData, freeEnergyPerturbationData,
                         globalCommunicationHelper->simulationSignals(),
@@ -401,7 +408,8 @@ ISimulatorElement* ComputeGlobalsElement<ComputeGlobalsAlgorithm::VelocityVerlet
                         vvComputeGlobalsElement);
         globalCommunicationHelper->setCheckBondedInteractionsCallback(
                 castedElement->getCheckNumberOfBondedInteractionsCallback());
+        builderHelper->storeValue(key, vvComputeGlobalsElement);
+        return vvComputeGlobalsElement;
     }
-    return vvComputeGlobalsElement;
 }
 } // namespace gmx

diff --git a/src/gromacs/modularsimulator/computeglobalselement.h b/src/gromacs/modularsimulator/computeglobalselement.h
index 28f0dbb5c9d099a657831a626d53c0b25aa5b420..b949145703a85b7f42c6030d6a3014dad1367e6e 100644 (file)
--- a/src/gromacs/modularsimulator/computeglobalselement.h
+++ b/src/gromacs/modularsimulator/computeglobalselement.h
@@ -154,6 +154,9 @@ public:
      * \param freeEnergyPerturbationData  Pointer to the \c FreeEnergyPerturbationData object
      * \param globalCommunicationHelper  Pointer to the \c GlobalCommunicationHelper object
      *
+     * \throws std::bad_any_cast  on internal error in VelocityVerlet algorithm builder.
+     * \throws std::bad_alloc  when out of memory.
+     *
      * \return  Pointer to the element to be added. Element needs to have been stored using \c storeElement
      */
     static ISimulatorElement* getElementPointerImpl(LegacySimulatorData* legacySimulatorData,

diff --git a/src/gromacs/modularsimulator/simulatoralgorithm.cpp b/src/gromacs/modularsimulator/simulatoralgorithm.cpp
index cf6061054be85155bc76800e3e361bb54b55ef84..4e0b71a9187b1ae4c33cdf5feae7d8b6867615f8 100644 (file)
--- a/src/gromacs/modularsimulator/simulatoralgorithm.cpp
+++ b/src/gromacs/modularsimulator/simulatoralgorithm.cpp
@@ -697,6 +697,19 @@ bool ModularSimulatorAlgorithmBuilderHelper::elementIsStored(const ISimulatorEle
     return builder_->elementExists(element);
 }
 
+std::optional<std::any> ModularSimulatorAlgorithmBuilderHelper::getStoredValue(const std::string& key) const
+{
+    const auto iter = values_.find(key);
+    if (iter == values_.end())
+    {
+        return std::nullopt;
+    }
+    else
+    {
+        return iter->second;
+    }
+}
+
 void ModularSimulatorAlgorithmBuilderHelper::registerThermostat(
         std::function<void(const PropagatorThermostatConnection&)> registrationFunction)
 {

diff --git a/src/gromacs/modularsimulator/simulatoralgorithm.h b/src/gromacs/modularsimulator/simulatoralgorithm.h
index 8073f0055f02d157bf4921394dbda6c8902367f7..e1e94f9150cb9b9b869c34bdc3fce135dc5e864e 100644 (file)
--- a/src/gromacs/modularsimulator/simulatoralgorithm.h
+++ b/src/gromacs/modularsimulator/simulatoralgorithm.h
@@ -48,10 +48,15 @@
 #ifndef GROMACS_MODULARSIMULATOR_SIMULATORALGORITHM_H
 #define GROMACS_MODULARSIMULATOR_SIMULATORALGORITHM_H
 
+#include <any>
+#include <map>
+#include <optional>
 #include <string>
+#include <typeinfo>
 
 #include "gromacs/mdrun/isimulator.h"
 #include "gromacs/mdtypes/state.h"
+#include "gromacs/utility/exceptions.h"
 
 #include "checkpointhelper.h"
 #include "domdechelper.h"
@@ -321,6 +326,11 @@ public:
     ISimulatorElement* storeElement(std::unique_ptr<ISimulatorElement> element);
     //! Check if an element is stored in the ModularSimulatorAlgorithmBuilder
     bool elementIsStored(const ISimulatorElement* element) const;
+    //! Set arbitrary data in the ModularSimulatorAlgorithmBuilder. Helpful for stateful elements.
+    template<typename ValueType>
+    void storeValue(const std::string& key, const ValueType& value);
+    //! Get previously stored data. Returns std::nullopt if key is not found.
+    std::optional<std::any> getStoredValue(const std::string& key) const;
     //! Register a thermostat that accepts propagator registrations
     void registerThermostat(std::function<void(const PropagatorThermostatConnection&)> registrationFunction);
     //! Register a barostat that accepts propagator registrations
@@ -333,6 +343,7 @@ public:
 private:
     //! Pointer to the associated ModularSimulatorAlgorithmBuilder
     ModularSimulatorAlgorithmBuilder* builder_;
+    std::map<std::string, std::any>   values_;
 };
 
 /*!\internal
@@ -605,6 +616,14 @@ void ModularSimulatorAlgorithmBuilder::registerWithInfrastructureAndSignallers(E
     checkpointHelperBuilder_.registerClient(castOrNull<ICheckpointHelperClient, Element>(element));
 }
 
+
+template<typename ValueType>
+void ModularSimulatorAlgorithmBuilderHelper::storeValue(const std::string& key, const ValueType& value)
+{
+    values_[key] = std::any(value);
+}
+
+
 } // namespace gmx
 
 #endif // GROMACS_MODULARSIMULATOR_SIMULATORALGORITHM_H

diff --git a/src/gromacs/modularsimulator/topologyholder.h b/src/gromacs/modularsimulator/topologyholder.h
index c6bb939e5fc1bf0d0efc6828e53f3f7eac0a3b58..335ab2b6e7785f91f00dbc6cc655221864fd9c80 100644 (file)
--- a/src/gromacs/modularsimulator/topologyholder.h
+++ b/src/gromacs/modularsimulator/topologyholder.h
@@ -93,7 +93,7 @@ public:
     class Builder;
 
 private:
-    //! Constant reference to the global topolgy
+    //! Constant reference to the global topology
     const gmx_mtop_t& globalTopology_;
     //! Pointer to the currently valid local topology
     std::unique_ptr<gmx_localtop_t> localTopology_;