SedInstanceTask reads the document's model changes on the run thread, racing with SedModel::addChange()/removeAllChanges()

VERSION.txt

@@ -1 +1 @@
-1.20261014.0
+1.20261015.0

src/api/libopencor/sedinstance.h

@@ -85,7 +85,8 @@ class LIBOPENCOR_EXPORT SedInstance: public Logger
      *
      * Start running, in a background thread, all the tasks associated with this instance. The results of the tasks
      * are (re)allocated before this method returns, so they can be retrieved while the tasks are being run (e.g., to
-     * plot them progressively). The simulation settings used are those in effect when this method is called.
+     * plot them progressively). The simulation settings and model changes used are those in effect when this method is
+     * called.
      *
      * @return @c true if a new run was started, @c false if a run is already in progress.
      */

src/sed/sedchangeattribute.cpp

@@ -87,12 +87,19 @@ void SedChangeAttribute::Impl::serialise(xmlNodePtr pNode) const
     xmlAddChild(pNode, node);
 }
 
-void SedChangeAttribute::Impl::apply(const SedInstanceTaskPtr &pInstanceTask,
-                                     const libcellml::AnalyserModelPtr &pAnalyserModel)
+void SedChangeAttribute::Impl::apply(SedInstanceTask::Impl *pInstanceTaskPimpl,
+                                     const libcellml::AnalyserModelPtr &pAnalyserModel) const
 {
-    auto addCannotChangeWarning = [this](const std::string &pVariableName,
-                                         const std::string &pComponentName,
-                                         const char *pVariableType) {
+    // Note: we report our warnings to the instance task rather than to ourselves. Indeed, we are owned by a model,
+    //       which may be shared by several instance tasks, and our warnings would otherwise accumulate every time we
+    //       get applied.
+
+    auto addInstanceTaskWarning = [pInstanceTaskPimpl](const std::string &pWarning) {
+        pInstanceTaskPimpl->addIssue(Issue::Type::WARNING, pWarning, "Change attribute");
+    };
+    auto addCannotChangeWarning = [&addInstanceTaskWarning](const std::string &pVariableName,
+                                                            const std::string &pComponentName,
+                                                            const char *pVariableType) {
         std::string warning;
 
         warning.reserve(pVariableName.size() + pComponentName.size() + 120); // NOLINT
@@ -105,17 +112,16 @@ void SedChangeAttribute::Impl::apply(const SedInstanceTaskPtr &pInstanceTask,
         warning += pComponentName;
         warning += "' cannot be changed. Only state variables and constants can be changed.";
 
-        addWarning(warning);
+        addInstanceTaskWarning(warning);
     };
 
-    auto *instanceTaskPimpl {pInstanceTask->pimpl()};
     auto changeName {name(mComponentName, mVariableName)};
-    const auto stateCount = instanceTaskPimpl->stateCount();
-    const auto constantCount = instanceTaskPimpl->constantCount();
-    const auto computedConstantCount = instanceTaskPimpl->computedConstantCount();
-    const auto algebraicVariableCount = instanceTaskPimpl->algebraicVariableCount();
+    const auto stateCount = pInstanceTaskPimpl->stateCount();
+    const auto constantCount = pInstanceTaskPimpl->constantCount();
+    const auto computedConstantCount = pInstanceTaskPimpl->computedConstantCount();
+    const auto algebraicVariableCount = pInstanceTaskPimpl->algebraicVariableCount();
 
-    if (instanceTaskPimpl->voiName() == changeName) {
+    if (pInstanceTaskPimpl->voiName() == changeName) {
         auto voiVariable {pAnalyserModel->voi()->variable()};
         auto voiComponent {owningComponent(voiVariable)};
 
@@ -127,8 +133,8 @@ void SedChangeAttribute::Impl::apply(const SedInstanceTaskPtr &pInstanceTask,
     auto isParameterSet {false};
 
     for (size_t i {0}; i < stateCount; ++i) {
-        if (instanceTaskPimpl->stateName(i) == changeName) {
-            instanceTaskPimpl->mChanges.emplace_back(instanceTaskPimpl->mStates + i, toDouble(newValue())); // NOLINT
+        if (pInstanceTaskPimpl->stateName(i) == changeName) {
+            pInstanceTaskPimpl->mChanges.emplace_back(pInstanceTaskPimpl->mStates + i, toDouble(newValue())); // NOLINT
 
             isParameterSet = true;
 
@@ -138,8 +144,8 @@ void SedChangeAttribute::Impl::apply(const SedInstanceTaskPtr &pInstanceTask,
 
     if (!isParameterSet) {
         for (size_t i {0}; i < constantCount; ++i) {
-            if (instanceTaskPimpl->constantName(i) == changeName) {
-                instanceTaskPimpl->mChanges.emplace_back(instanceTaskPimpl->mConstants + i, toDouble(newValue())); // NOLINT
+            if (pInstanceTaskPimpl->constantName(i) == changeName) {
+                pInstanceTaskPimpl->mChanges.emplace_back(pInstanceTaskPimpl->mConstants + i, toDouble(newValue())); // NOLINT
 
                 isParameterSet = true;
 
@@ -152,7 +158,7 @@ void SedChangeAttribute::Impl::apply(const SedInstanceTaskPtr &pInstanceTask,
         const auto &computedConstants = pAnalyserModel->computedConstants();
 
         for (size_t i {0}; i < computedConstantCount; ++i) {
-            if (instanceTaskPimpl->computedConstantName(i) == changeName) {
+            if (pInstanceTaskPimpl->computedConstantName(i) == changeName) {
                 auto computedConstantVariable {computedConstants[i]->variable()};
                 auto computedConstantComponent {owningComponent(computedConstantVariable)};
 
@@ -169,7 +175,7 @@ void SedChangeAttribute::Impl::apply(const SedInstanceTaskPtr &pInstanceTask,
         const auto &algebraicVariables = pAnalyserModel->algebraicVariables();
 
         for (size_t i {0}; i < algebraicVariableCount; ++i) {
-            if (instanceTaskPimpl->algebraicVariableName(i) == changeName) {
+            if (pInstanceTaskPimpl->algebraicVariableName(i) == changeName) {
                 auto algebraicVariable {algebraicVariables[i]->variable()};
                 auto algebraicComponent {owningComponent(algebraicVariable)};
 
@@ -193,7 +199,7 @@ void SedChangeAttribute::Impl::apply(const SedInstanceTaskPtr &pInstanceTask,
         warning += mComponentName;
         warning += "' could not be found and therefore could not be changed.";
 
-        addWarning(warning);
+        addInstanceTaskWarning(warning);
     }
 }
 

src/sed/sedchangeattribute_p.h

@@ -46,7 +46,7 @@ class SedChangeAttribute::Impl: public SedChange::Impl
 
     void serialise(xmlNodePtr pNode) const override;
 
-    void apply(const SedInstanceTaskPtr &pInstanceTask, const libcellml::AnalyserModelPtr &pAnalyserModel);
+    void apply(SedInstanceTask::Impl *pInstanceTaskPimpl, const libcellml::AnalyserModelPtr &pAnalyserModel) const;
 };
 
 } // namespace libOpenCOR

src/sed/sedinstance.cpp

@@ -59,6 +59,8 @@ SedInstance::Impl::Impl(const SedDocumentPtr &pDocument)
         }
 
         // Create an instance of all the tasks, if they are all valid.
+        // Note: a task instance that has some issues (e.g., its model could not be compiled) cannot be run (see
+        //       prepareRun()), hence its issues are reported and kept track of (see below).
 
         if (tasksValid) {
             mTasks.reserve(tasks.size());
@@ -68,9 +70,11 @@ SedInstance::Impl::Impl(const SedDocumentPtr &pDocument)
 
                 mTasks.push_back(taskInstance);
 
+#ifndef CODE_COVERAGE_ENABLED
                 if (taskInstance->hasIssues()) {
                     addIssues(taskInstance, "Task instance");
                 }
+#endif
             }
         }
     } else {
@@ -83,6 +87,31 @@ SedInstance::Impl::Impl(const SedDocumentPtr &pDocument)
     mTasksIssues = mIssues;
     mTasksErrors = mErrors;
     mTasksWarnings = mWarnings;
+
+    // Initialise the task instances that have no issues, so that we can report the issues with their changes (e.g., a
+    // change that cannot be applied) and their initialisation (e.g., an NLA system that cannot be solved), and reset
+    // those issues so that they don't prevent those task instances from being run.
+    // Note: those issues are not kept track of since they depend on the changes to the model and on the simulation
+    //       settings, both of which may be changed before the instance is run. Instead, the changes of a task instance
+    //       are retrieved again and the task instance is initialised again every time it is run, at which point those
+    //       issues get reported again, if still relevant (see prepareRun() and executeRun()).
+
+    for (const auto &task : mTasks) {
+#ifndef CODE_COVERAGE_ENABLED
+        if (!task->hasIssues()) {
+#endif
+            auto *taskPimpl {task->pimpl()};
+
+            taskPimpl->retrieveChanges();
+            taskPimpl->initialise();
+
+            addIssues(task, "Task instance");
+
+            taskPimpl->removeAllIssues();
+#ifndef CODE_COVERAGE_ENABLED
+        }
+#endif
+    }
 }
 
 SedInstance::Status SedInstance::Impl::status() const
@@ -140,9 +169,12 @@ void SedInstance::Impl::prepareRun()
     }
 
     // Prepare all the tasks associated with this instance to be run unless they have some issues.
-    // Note: a task may throw an exception (e.g., std::bad_alloc if its results cannot be allocated), in which case we
-    //       report it as an error rather than let it escape and we don't run any of our tasks. We have no way to
-    //       trigger such an exception in our tests, hence we ignore our try...catch statement during code coverage.
+    // Note #1: a task may throw an exception (e.g., std::bad_alloc if its results cannot be allocated), in which case
+    //          we report it as an error rather than let it escape and we don't run any of our tasks. We have no way to
+    //          trigger such an exception in our tests, hence we ignore our try...catch statement during code coverage.
+    // Note #2: a task only has issues at this stage if its instance could not be created (e.g., its model could not be
+    //          compiled, see Impl()), which we have no way to trigger in our tests, hence we ignore our check during
+    //          code coverage.
 
     mTasksToRun.clear();
 
@@ -152,18 +184,27 @@ void SedInstance::Impl::prepareRun()
         mTasksToRun.reserve(mTasks.size());
 
         for (const auto &task : mTasks) {
+#ifndef CODE_COVERAGE_ENABLED
             if (!task->hasIssues()) {
-                if (task->pimpl()->prepareRun()) {
-                    mTasksToRun.push_back(task);
-                } else {
-                    addIssues(task, "Task");
+#endif
+                const auto taskPrepared {task->pimpl()->prepareRun()};
 
-                    // Reset the issues of the task so that they are not reported again should the instance be run
-                    // again.
+                // Report the issues of the task, if any (i.e. the errors about its simulation or the warnings about the
+                // changes that could not be applied to its model), and reset them so that they are not reported again
+                // should the instance be run again and so that warnings don't prevent the task from being run.
+
+                if (task->hasIssues()) {
+                    addIssues(task, "Task");
 
                     task->pimpl()->removeAllIssues();
                 }
+
+                if (taskPrepared) {
+                    mTasksToRun.push_back(task);
+                }
+#ifndef CODE_COVERAGE_ENABLED
             }
+#endif
         }
 #ifndef CODE_COVERAGE_ENABLED
     } catch (const std::exception &exception) {

src/sed/sedinstancetask.cpp

@@ -56,21 +56,7 @@ static emscripten::val toFloat64Array(std::span<const double> data)
 
 SedInstanceTaskPtr SedInstanceTask::Impl::create(const SedAbstractTaskPtr &pTask)
 {
-    auto res {SedInstanceTaskPtr {new SedInstanceTask {pTask}}};
-
-    res->pimpl()->mOwner = res;
-
-    // Initialise the instance task, but only if there are no issues with it.
-
-#ifndef CODE_COVERAGE_ENABLED
-    if (!res->hasIssues()) {
-#endif
-        res->pimpl()->initialise();
-#ifndef CODE_COVERAGE_ENABLED
-    }
-#endif
-
-    return res;
+    return SedInstanceTaskPtr {new SedInstanceTask {pTask}};
 }
 
 SedInstanceTask::Impl::Impl(const SedAbstractTaskPtr &pTask)
@@ -363,9 +349,14 @@ void SedInstanceTask::Impl::nanFillResults(size_t pFromIndex)
     nanFillRowTails(mResults.algebraicVariables, mAlgebraicVariableCount);
 }
 
-void SedInstanceTask::Impl::applyChanges()
+void SedInstanceTask::Impl::retrieveChanges()
 {
     // Retrieve our changes (see SedChangeAttribute::Impl::apply()).
+    // Note: we are called from the thread that creates our instance or that runs us or starts running us (see
+    //       SedInstance::Impl::Impl() and prepareRun()), i.e. before we actually get run. This means that our changes
+    //       are those that were in effect when we were asked to be run, even if they get changed while we are being
+    //       run. It also means that we don't access our model while we are being run, so our model can safely be
+    //       changed while we are being run.
 
     mChanges.clear();
 
@@ -377,17 +368,18 @@ void SedInstanceTask::Impl::applyChanges()
 
         ASSERT_NE(changeAttribute, nullptr);
 
-        changeAttribute->pimpl()->apply(mOwner.lock(), mAnalyserModel);
-
-        addIssues(changeAttribute, "Change attribute");
+        changeAttribute->pimpl()->apply(this, mAnalyserModel);
     }
+}
 
-    // Apply our changes, which means setting our changed variables and reinitialising the variables that are
-    // initialised using another variable since our generated code initialised them before we could apply our changes.
-    // We need to do this both before computing our computed constants (since they may depend on a changed variable or
-    // a variable that is initialised using another variable) and after (since our generated code initialises a
-    // variable that depends on a computed constant when computing our computed constants, which means that it may
-    // override a change to that variable).
+void SedInstanceTask::Impl::applyChanges()
+{
+    // Apply our changes (see retrieveChanges()), which means setting our changed variables and reinitialising the
+    // variables that are initialised using another variable since our generated code initialised them before we could
+    // apply our changes. We need to do this both before computing our computed constants (since they may depend on a
+    // changed variable or a variable that is initialised using another variable) and after (since our generated code
+    // initialises a variable that depends on a computed constant when computing our computed constants, which means
+    // that it may override a change to that variable).
 
     auto isChangedVariable = [this](const double *pVariable) {
         return std::ranges::any_of(mChanges, [pVariable](const auto &pChange) {
@@ -600,8 +592,8 @@ bool SedInstanceTask::Impl::prepareRun()
     // Note: we are called from the thread that runs us or that starts running us (see SedInstance::Impl::prepareRun()),
     //       i.e. before we actually get run. This means that our results are (re)allocated before we get run, so that
     //       they can be safely retrieved while we are being run (e.g., to plot them progressively). This also means
-    //       that our simulation settings are those that were in effect when we were asked to be run, even if they get
-    //       changed while we are being run.
+    //       that our simulation settings and model changes are those that were in effect when we were asked to be
+    //       run, even if they get changed while we are being run.
 
     // Reset our progress counters.
 
@@ -638,6 +630,14 @@ bool SedInstanceTask::Impl::prepareRun()
 
     allocateResults(mDifferentialModel ? mNumberOfSteps + 1 : 1);
 
+    // Retrieve our changes.
+    // Note: this must be done after (re)allocating our results. Indeed, retrieving our changes may result in some
+    //       warnings (e.g., a change that cannot be applied) and should the (re)allocation of our results then throw an
+    //       exception, those warnings would never be reported nor reset (see SedInstance::Impl::prepareRun()), which
+    //       would prevent us from ever being run again.
+
+    retrieveChanges();
+
     // Set our total number of steps.
 
     mTotalSteps.store(mNumberOfSteps, std::memory_order_relaxed);
@@ -654,7 +654,8 @@ double SedInstanceTask::Impl::run()
     auto startTime {std::chrono::high_resolution_clock::now()};
 
     // (Re)initialise our model.
-    // Note: reinitialise our model because we initialised it when we created the instance task.
+    // Note: reinitialise our model because it was initialised when our instance was created (see
+    //       SedInstance::Impl::Impl()).
 
     initialise();
 

src/sed/sedinstancetask_p.h

@@ -64,12 +64,9 @@ struct SedInstanceTaskInitialisation
 using SedInstanceTaskInitialisations = std::vector<SedInstanceTaskInitialisation>;
 using SedInstanceTaskChanges = std::vector<std::pair<double *, double>>;
 
-using SedInstanceTaskWeakPtr = std::weak_ptr<SedInstanceTask>;
-
 class SedInstanceTask::Impl: public Logger::Impl
 {
 public:
-    SedInstanceTaskWeakPtr mOwner;
     CellmlFileRuntimePtr mRuntime;
     SedSimulationPtr mSimulation;
     SedUniformTimeCoursePtr mSedUniformTimeCourse;
@@ -136,6 +133,7 @@ class SedInstanceTask::Impl: public Logger::Impl
     void trackResults(size_t pIndex);
     void nanFillResults(size_t pFromIndex);
 
+    void retrieveChanges();
     void applyChanges();
     void initialise();
     bool prepareRun();

tests/api/sed/coveragetests.cpp

@@ -556,9 +556,12 @@ TEST(CoverageSedTest, sedUniformTimeCourse)
 
 TEST(CoverageSedTest, sedInstanceAndSedInstanceTaskDifferentialModel)
 {
-    static const libOpenCOR::ExpectedIssues EXPECTED_ISSUES {{
+    static const libOpenCOR::ExpectedIssues EXPECTED_INSTANTIATION_ISSUES {{
         {libOpenCOR::Issue::Type::ERROR, "Task instance | CVODE: the upper half-bandwidth cannot be equal to -1. It must be between 0 and 3."},
     }};
+    static const libOpenCOR::ExpectedIssues EXPECTED_RUN_ISSUES {{
+        {libOpenCOR::Issue::Type::ERROR, "Task | CVODE: the upper half-bandwidth cannot be equal to -1. It must be between 0 and 3."},
+    }};
     static const auto UPPER_HALF_BANDWIDTH {-1};
 
     auto file {libOpenCOR::File::create(libOpenCOR::resourcePath("api/solver/ode.cellml"))};
@@ -571,6 +574,8 @@ TEST(CoverageSedTest, sedInstanceAndSedInstanceTaskDifferentialModel)
     auto instance {document->instantiate()};
     const auto &instanceTask {instance->tasks()[0]};
 
+    EXPECT_EQ_ISSUES(instance, EXPECTED_INSTANTIATION_ISSUES);
+
     EXPECT_EQ(instance->hasTasks(), true);
     EXPECT_EQ(instance->taskCount(), 1U);
     EXPECT_EQ(instance->task(0), instanceTask);
@@ -622,7 +627,7 @@ TEST(CoverageSedTest, sedInstanceAndSedInstanceTaskDifferentialModel)
 
     instance->run();
 
-    EXPECT_EQ_ISSUES(instance, EXPECTED_ISSUES);
+    EXPECT_EQ_ISSUES(instance, EXPECTED_RUN_ISSUES);
 }
 
 TEST(CoverageSedTest, sedInstanceAndSedInstanceTaskNonDifferentialModel)