Clarify behavior of LegacyMdrunOptions.

Make notes about the lifetime management of LegacyMdrunOptions and its
members. Note effects on the way it must be used by gmxapi::ContextImpl.
Clarify how gmxapi::ContextImpl could be updated in the future if data
scope details of LegacyMdrunOptions evolve.

Relates to #3644, #3688, #3718

diff --git a/api/gmxapi/cpp/context.cpp b/api/gmxapi/cpp/context.cpp
index 903f3ce56d6ba6fcc24eff3359e56ffb1821928b..1b5cb813d14bbf0812796d074280f99e0b66ac99 100644 (file)
--- a/api/gmxapi/cpp/context.cpp
+++ b/api/gmxapi/cpp/context.cpp
@@ -315,13 +315,18 @@ std::shared_ptr<Session> ContextImpl::launch(const Workflow& work)
         auto mdModules = std::make_unique<MDModules>();
 
         const char* desc[] = { "gmxapi placeholder text" };
-        if (options_.updateFromCommandLine(argc, argv.data(), desc) == 0)
+
+        // LegacyMdrunOptions needs to be kept alive for the life of ContextImpl,
+        // so we use a data member for now.
+        gmx::LegacyMdrunOptions& options = options_;
+        if (options.updateFromCommandLine(argc, argv.data(), desc) == 0)
         {
             return nullptr;
         }
 
         ArrayRef<const std::string> multiSimDirectoryNames =
-                opt2fnsIfOptionSet("-multidir", ssize(options_.filenames), options_.filenames.data());
+                opt2fnsIfOptionSet("-multidir", ssize(options.filenames), options.filenames.data());
+
 
         // The SimulationContext is necessary with gmxapi so that
         // resources owned by the client code can have suitable
@@ -335,39 +340,39 @@ std::shared_ptr<Session> ContextImpl::launch(const Workflow& work)
         SimulationContext simulationContext(communicator, multiSimDirectoryNames);
 
 
-        StartingBehavior startingBehavior = StartingBehavior::NewSimulation;
-        LogFilePtr       logFileGuard     = nullptr;
-        gmx_multisim_t*  ms               = simulationContext.multiSimulation_.get();
-        std::tie(startingBehavior, logFileGuard) =
-                handleRestart(findIsSimulationMasterRank(ms, simulationContext.simulationCommunicator_),
-                              communicator, ms, options_.mdrunOptions.appendingBehavior,
-                              ssize(options_.filenames), options_.filenames.data());
+        StartingBehavior startingBehavior        = StartingBehavior::NewSimulation;
+        LogFilePtr       logFileGuard            = nullptr;
+        gmx_multisim_t*  ms                      = simulationContext.multiSimulation_.get();
+        std::tie(startingBehavior, logFileGuard) = handleRestart(
+                findIsSimulationMasterRank(ms, simulationContext.simulationCommunicator_),
+                simulationContext.simulationCommunicator_, ms, options.mdrunOptions.appendingBehavior,
+                ssize(options.filenames), options.filenames.data());
 
         auto builder = MdrunnerBuilder(std::move(mdModules),
                                        compat::not_null<SimulationContext*>(&simulationContext));
-        builder.addSimulationMethod(options_.mdrunOptions, options_.pforce, startingBehavior);
-        builder.addDomainDecomposition(options_.domdecOptions);
+        builder.addSimulationMethod(options.mdrunOptions, options.pforce, startingBehavior);
+        builder.addDomainDecomposition(options.domdecOptions);
         // \todo pass by value
-        builder.addNonBonded(options_.nbpu_opt_choices[0]);
+        builder.addNonBonded(options.nbpu_opt_choices[0]);
         // \todo pass by value
-        builder.addElectrostatics(options_.pme_opt_choices[0], options_.pme_fft_opt_choices[0]);
-        builder.addBondedTaskAssignment(options_.bonded_opt_choices[0]);
-        builder.addUpdateTaskAssignment(options_.update_opt_choices[0]);
-        builder.addNeighborList(options_.nstlist_cmdline);
-        builder.addReplicaExchange(options_.replExParams);
+        builder.addElectrostatics(options.pme_opt_choices[0], options.pme_fft_opt_choices[0]);
+        builder.addBondedTaskAssignment(options.bonded_opt_choices[0]);
+        builder.addUpdateTaskAssignment(options.update_opt_choices[0]);
+        builder.addNeighborList(options.nstlist_cmdline);
+        builder.addReplicaExchange(options.replExParams);
         // Need to establish run-time values from various inputs to provide a resource handle to Mdrunner
-        builder.addHardwareOptions(options_.hw_opt);
+        builder.addHardwareOptions(options.hw_opt);
 
         // \todo File names are parameters that should be managed modularly through further factoring.
-        builder.addFilenames(options_.filenames);
+        builder.addFilenames(options.filenames);
         // TODO: Remove `s` and `-cpi` from LegacyMdrunOptions before launch(). #3652
-        auto simulationInput = makeSimulationInput(options_);
+        auto simulationInput = makeSimulationInput(options);
         builder.addInput(simulationInput);
 
         // Note: The gmx_output_env_t life time is not managed after the call to parse_common_args.
         // \todo Implement lifetime management for gmx_output_env_t.
         // \todo Output environment should be configured outside of Mdrunner and provided as a resource.
-        builder.addOutputEnvironment(options_.oenv);
+        builder.addOutputEnvironment(options.oenv);
         builder.addLogFile(logFileGuard.get());
 
         // Note, creation is not mature enough to be exposed in the external API yet.

diff --git a/src/gromacs/mdrun/legacymdrunoptions.h b/src/gromacs/mdrun/legacymdrunoptions.h
index 796e4794903989e848b5fe774b156cf1ef90494a..474f6f0396258f227def1cc012ec38bdeddca284 100644 (file)
--- a/src/gromacs/mdrun/legacymdrunoptions.h
+++ b/src/gromacs/mdrun/legacymdrunoptions.h
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 1991-2000, University of Groningen, The Netherlands.
  * Copyright (c) 2001-2004, The GROMACS development team.
- * Copyright (c) 2011-2019, by the GROMACS development team, led by
+ * Copyright (c) 2011-2020, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -68,13 +68,19 @@ namespace gmx
  * support for the CLI and API without duplicating code. It should be
  * eliminated following the TODOs below.
  *
+ * \warning Instances provide lifetime scope for members that do not have
+ *  effective lifetime management or which are frequently accessed unsafely.
+ *  The caller is responsible for keeping a LegacyMdrunOptions object alive
+ *  for as long as any consumers, direct or transitive.
+ *
  * \todo Modules in mdrun should acquire proper option handling so
- * that all of these declarations and defaults are local to the
- * modules.
+ *       that all of these declarations and defaults are local to the
+ *       modules.
  *
  * \todo Contextual aspects, such as working directory
- * and environment variable handling are more properly
- * the role of SimulationContext, and should be moved there */
+ *       and environment variable handling are more properly
+ *       the role of SimulationContext, and should be moved there.
+ */
 class LegacyMdrunOptions
 {
 public: