SedInstance: a run that throws leaves the instance stuck in RUNNING and deleting it calls std::terminate()

VERSION.txt

@@ -1 +1 @@
-1.20261008.1
+1.20261009.0

src/sed/sedinstance.cpp

@@ -21,6 +21,7 @@ limitations under the License.
 #include "libopencor/seddocument.h"
 
 #include <chrono>
+#include <exception>
 #include <memory>
 
 namespace libOpenCOR {
@@ -140,22 +141,38 @@ double SedInstance::Impl::run()
     }
 
     // Run all the tasks associated with this instance 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. Indeed, run() may be called from startRun(), i.e. on a
+    //       separate thread, and an escaping exception would leave us in a state from which we cannot recover. Also,
+    //       we have no way to trigger such an exception in our tests, hence we ignore our try...catch statement during
+    //       code coverage.
 
     auto res {0.0};
 
-    for (const auto &task : mTasks) {
-        if (!task->hasIssues()) {
-            res += task->pimpl()->run();
+#ifndef CODE_COVERAGE_ENABLED
+    try {
+#endif
+        for (const auto &task : mTasks) {
+            if (!task->hasIssues()) {
+                res += task->pimpl()->run();
 
-            if (task->hasIssues()) {
-                addIssues(task, "Task");
+                if (task->hasIssues()) {
+                    addIssues(task, "Task");
 
-                // Reset the issues of the task so that they are not reported again should the instance be run again.
+                    // Reset the issues of the task so that they are not reported again should the instance be run
+                    // again.
 
-                task->pimpl()->removeAllIssues();
+                    task->pimpl()->removeAllIssues();
+                }
             }
         }
+#ifndef CODE_COVERAGE_ENABLED
+    } catch (const std::exception &exception) {
+        addError(std::string("The simulation failed: ") + exception.what() + ".");
+    } catch (...) {
+        addError("The simulation failed.");
     }
+#endif
 
     // Reset and make sure that our control flags are no longer passed to each task.
 
@@ -275,7 +292,20 @@ SedInstance::SedInstance(const SedDocumentPtr &pDocument)
 
 SedInstance::~SedInstance()
 {
-    pimpl()->waitForRun(); // To ensure that the instance is not running before we delete it.
+    // Make sure that the instance is not running before we delete it.
+    // Note: run() reports a failure as an issue rather than throw an exception, so waitForRun() should never throw, but
+    //       an exception must never escape a destructor (it would result in std::terminate() being called), hence we
+    //       make sure of it.
+
+#ifndef CODE_COVERAGE_ENABLED
+    try {
+#endif
+        pimpl()->waitForRun();
+#ifndef CODE_COVERAGE_ENABLED
+    } catch (...) { // NOLINT(bugprone-empty-catch)
+        // There is nothing more that we can do since we are being deleted.
+    }
+#endif
 }
 
 SedInstance::Impl *SedInstance::pimpl()

src/sed/sedmodel.cpp

@@ -41,6 +41,12 @@ const FilePtr &SedModel::Impl::file() const
 
 bool SedModel::Impl::isValid()
 {
+    // Reset our issues so that they are not reported again should we be validated again.
+
+    removeAllIssues();
+
+    // Make sure that the CellML file is valid.
+
     switch (mFile->pimpl()->mCellmlFile->type()) {
     case libcellml::AnalyserModel::Type::INVALID:
         addError("The CellML file is invalid.");

src/sed/sedsimulation.cpp

@@ -18,9 +18,12 @@ limitations under the License.
 #include "seddocument_p.h"
 #include "sedmodel_p.h"
 #include "sedsimulation_p.h"
+#include "seduniformtimecourse_p.h"
 #include "solvernla_p.h"
 #include "solverode_p.h"
 
+#include "utils.h"
+
 namespace libOpenCOR {
 
 static constexpr auto ID_PREFIX {"simulation"};
@@ -30,7 +33,7 @@ SedSimulation::Impl::Impl(const SedDocumentPtr &pDocument)
 {
 }
 
-bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, bool pUniformTimeCourse)
+bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, const SedUniformTimeCoursePtr &pUniformTimeCourse)
 {
     auto modelType {pModel->pimpl()->mFile->pimpl()->mCellmlFile->type()};
     const auto &modelId = pModel->pimpl()->mId;
@@ -51,23 +54,44 @@ bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, bool pUniformTimeCo
         addError(error);
     };
 
-    // Make sure that we are a uniform time course simulation if our model is an ODE/DAE model.
+    // Reset our issues so that they are not reported again should we be validated again.
+
+    removeAllIssues();
+
+    // Make sure that we are a uniform time course simulation with a strictly positive number of steps if our model is
+    // an ODE/DAE model.
     //---GRY--- WE DON'T CURRENTLY SUPPORT OTHER TYPES OF SIMULATION FOR ODE/DAE MODELS.
 
-    if (((modelType == libcellml::AnalyserModel::Type::ODE)
-         || (modelType == libcellml::AnalyserModel::Type::DAE))
-        && !pUniformTimeCourse) {
-        std::string error;
+    if ((modelType == libcellml::AnalyserModel::Type::ODE)
+        || (modelType == libcellml::AnalyserModel::Type::DAE)) {
+        if (pUniformTimeCourse == nullptr) {
+            std::string error;
 
-        error.reserve(mId.size() + modelId.size() + 96); // NOLINT
+            error.reserve(mId.size() + modelId.size() + 96); // NOLINT
 
-        error += "Simulation '";
-        error += mId;
-        error += "' is to be used with model '";
-        error += modelId;
-        error += "' which (currently) requires a uniform time course simulation.";
+            error += "Simulation '";
+            error += mId;
+            error += "' is to be used with model '";
+            error += modelId;
+            error += "' which (currently) requires a uniform time course simulation.";
 
-        addError(error);
+            addError(error);
+        } else if (pUniformTimeCourse->numberOfSteps() <= 0) {
+            const auto numberOfSteps {toString(pUniformTimeCourse->numberOfSteps())};
+            std::string error;
+
+            error.reserve(mId.size() + modelId.size() + numberOfSteps.size() + 112); // NOLINT
+
+            error += "Simulation '";
+            error += mId;
+            error += "' is to be used with model '";
+            error += modelId;
+            error += "' which requires a strictly positive number of steps but ";
+            error += numberOfSteps;
+            error += " is provided.";
+
+            addError(error);
+        }
     }
 
     // Make sure that we have the solver(s) required by our model.

src/sed/sedsimulation_p.h

@@ -30,7 +30,7 @@ class SedSimulation::Impl: public SedBase::Impl
 
     explicit Impl(const SedDocumentPtr &pDocument);
 
-    bool isValid(const SedModelPtr &pModel, bool pUniformTimeCourse);
+    bool isValid(const SedModelPtr &pModel, const SedUniformTimeCoursePtr &pUniformTimeCourse);
 
     const SolverOdePtr &odeSolver() const;
     void setOdeSolver(const SolverOdePtr &pOdeSolver);

src/sed/sedtask.cpp

@@ -79,7 +79,7 @@ bool SedTask::Impl::isValid()
 
     // Make sure that the simulation is valid for the model.
 
-    if (!mSimulation->pimpl()->isValid(mModel, std::dynamic_pointer_cast<SedUniformTimeCourse>(mSimulation) != nullptr)) {
+    if (!mSimulation->pimpl()->isValid(mModel, std::dynamic_pointer_cast<SedUniformTimeCourse>(mSimulation))) {
         addIssues(mSimulation, "Simulation");
     }
 

tests/api/sed/instancetests.cpp

@@ -634,6 +634,49 @@ TEST(InstanceSedTest, odeModelWithNonUniformTimeCourseSimulation)
     EXPECT_EQ_ISSUES(instance, STEADY_STATE_EXPECTED_ISSUES);
 }
 
+TEST(InstanceSedTest, odeModelWithInvalidNumberOfSteps)
+{
+    static const libOpenCOR::ExpectedIssues ZERO_STEPS_EXPECTED_ISSUES {{
+        {libOpenCOR::Issue::Type::ERROR, "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but 0 is provided."},
+    }};
+    static const libOpenCOR::ExpectedIssues NEGATIVE_STEPS_EXPECTED_ISSUES {{
+        {libOpenCOR::Issue::Type::ERROR, "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but -100 is provided."},
+    }};
+
+    auto file {libOpenCOR::File::create(libOpenCOR::resourcePath("cellml_2.cellml"))};
+    auto document {libOpenCOR::SedDocument::create(file)};
+    const auto &simulation {std::dynamic_pointer_cast<libOpenCOR::SedUniformTimeCourse>(document->simulations()[0])};
+
+    simulation->setNumberOfSteps(0);
+
+    auto instance {document->instantiate()};
+
+    EXPECT_EQ_ISSUES(instance, ZERO_STEPS_EXPECTED_ISSUES);
+
+    simulation->setNumberOfSteps(-100); // NOLINT
+
+    instance = document->instantiate();
+
+    EXPECT_EQ_ISSUES(instance, NEGATIVE_STEPS_EXPECTED_ISSUES);
+
+    // Running the instance should not do anything, but it should still leave it idle.
+
+    EXPECT_TRUE(instance->startRun());
+    EXPECT_EQ(instance->waitForRun(), 0.0);
+    EXPECT_EQ(instance->status(), libOpenCOR::SedInstance::Status::IDLE);
+    EXPECT_EQ_ISSUES(instance, NEGATIVE_STEPS_EXPECTED_ISSUES);
+
+    // Make sure that the number of steps is not reported as invalid anymore once it has been fixed.
+
+    simulation->setNumberOfSteps(1000); // NOLINT
+
+    instance = document->instantiate();
+
+    EXPECT_FALSE(instance->hasIssues());
+    EXPECT_GT(instance->run(), 0.0);
+    EXPECT_FALSE(instance->hasIssues());
+}
+
 TEST(InstanceSedTest, nlaModel)
 {
     static const libOpenCOR::ExpectedIssues EXPECTED_ISSUES {{

tests/bindings/javascript/sed.instance.test.js

@@ -693,6 +693,56 @@ test.describe('Sed instance tests', () => {
     ]);
   });
 
+  test('ODE model with invalid number of steps', () => {
+    const file = new loc.File(utils.resourcePath('cellml_2.cellml'));
+
+    file.setContents(utils.fileContents(file.path));
+
+    const document = new loc.SedDocument(file);
+    const simulation = document.simulations.get(0);
+
+    simulation.numberOfSteps = 0;
+
+    let instance = document.instantiate();
+
+    assertIssues(loc, instance, [
+      [
+        loc.Issue.Type.ERROR,
+        "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but 0 is provided."
+      ]
+    ]);
+
+    simulation.numberOfSteps = -100;
+
+    instance = document.instantiate();
+
+    const negativeStepsExpectedIssues = [
+      [
+        loc.Issue.Type.ERROR,
+        "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but -100 is provided."
+      ]
+    ];
+
+    assertIssues(loc, instance, negativeStepsExpectedIssues);
+
+    // Running the instance should not do anything, but it should still leave it idle.
+
+    assert.strictEqual(instance.startRun(), true);
+    assert.strictEqual(instance.waitForRun(), 0.0);
+    assert.strictEqual(instance.status, loc.SedInstance.Status.IDLE);
+    assertIssues(loc, instance, negativeStepsExpectedIssues);
+
+    // Make sure that the number of steps is not reported as invalid anymore once it has been fixed.
+
+    simulation.numberOfSteps = 1000;
+
+    instance = document.instantiate();
+
+    assert.strictEqual(instance.hasIssues, false);
+    assert.ok(instance.run() > 0.0);
+    assert.strictEqual(instance.hasIssues, false);
+  });
+
   test('NLA model', () => {
     const file = new loc.File(utils.resourcePath('api/sed/nla.cellml'));
 

tests/bindings/python/test_sed_instance.py

@@ -620,6 +620,54 @@ def test_ode_model_with_non_uniform_time_course_simulation():
     assert_issues(instance, steady_state_expected_issues)
 
 
+def test_ode_model_with_invalid_number_of_steps():
+    zero_steps_expected_issues = [
+        [
+            loc.Issue.Type.Error,
+            "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but 0 is provided.",
+        ],
+    ]
+    negative_steps_expected_issues = [
+        [
+            loc.Issue.Type.Error,
+            "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but -100 is provided.",
+        ],
+    ]
+
+    file = loc.File(utils.resource_path("cellml_2.cellml"))
+    document = loc.SedDocument(file)
+    simulation = document.simulations[0]
+
+    simulation.number_of_steps = 0
+
+    instance = document.instantiate()
+
+    assert_issues(instance, zero_steps_expected_issues)
+
+    simulation.number_of_steps = -100
+
+    instance = document.instantiate()
+
+    assert_issues(instance, negative_steps_expected_issues)
+
+    # Running the instance should not do anything, but it should still leave it idle.
+
+    assert instance.start_run() is True
+    assert instance.wait_for_run() == 0.0
+    assert instance.status == loc.SedInstance.Status.Idle
+    assert_issues(instance, negative_steps_expected_issues)
+
+    # Make sure that the number of steps is not reported as invalid anymore once it has been fixed.
+
+    simulation.number_of_steps = 1000
+
+    instance = document.instantiate()
+
+    assert not instance.has_issues
+    assert instance.run() > 0.0
+    assert not instance.has_issues
+
+
 def test_nla_model():
     expected_issues = [
         [