SedInstance: stopRun() called right after startRun() is ignored

VERSION.txt

@@ -1 +1 @@
-1.20261009.2
+1.20261009.3

src/api/libopencor/sedinstance.h

@@ -24,6 +24,9 @@ namespace libOpenCOR {
  * @brief The SedInstance class.
  *
  * The SedInstance class is used to describe an instance of a simulation experiment description.
+ *
+ * Deleting an instance stops any run in progress (be it running or paused) and waits for it to stop, so the results of
+ * a task that is kept beyond the deletion of its instance may be incomplete.
  */
 
 class LIBOPENCOR_EXPORT SedInstance: public Logger
@@ -115,7 +118,7 @@ class LIBOPENCOR_EXPORT SedInstance: public Logger
     /**
      * @brief Stop any currently-running instance.
      *
-     * Stop any currently-running instance.
+     * Stop any currently-running instance. This is also done when an instance is deleted.
      */
 
     void stopRun();

src/sed/sedinstance.cpp

@@ -130,9 +130,11 @@ double SedInstance::Impl::run()
         mWarningCount.store(mWarnings.size(), std::memory_order_release);
     }
 
-    // Reset our control flags and make sure that they are passed to each task so that they can be used by them.
-
-    mRunControl.store(INSTANCE_RUN_CONTROL_NONE, std::memory_order_relaxed);
+    // Make sure that our control flags are passed to each task so that they can be used by them.
+    // Note: our control flags are reset by our callers (see SedInstance::run() and startRun()) rather than here.
+    //       Indeed, when called from startRun(), we are run on a separate thread, i.e. some time after startRun() has
+    //       returned. So, if we were to reset our control flags here, a stop or pause requested in between would be
+    //       lost.
 
     for (const auto &task : mTasks) {
         task->pimpl()->mRunControl = &mRunControl;
@@ -199,6 +201,10 @@ bool SedInstance::Impl::startRun()
         mLastRunElapsedTime.store(mRunFuture.get(), std::memory_order_relaxed);
     }
 
+    // Reset our control flags (see the note in run()).
+
+    mRunControl.store(INSTANCE_RUN_CONTROL_NONE, std::memory_order_relaxed);
+
     mRunning.store(true, std::memory_order_release);
 
     // Start our run in a separate thread.
@@ -252,14 +258,28 @@ void SedInstance::Impl::pauseRun()
 
 void SedInstance::Impl::resumeRun()
 {
-    mRunControl.fetch_and(~INSTANCE_RUN_CONTROL_PAUSE, std::memory_order_relaxed);
+    // Note: our control flags must be updated while holding our pause mutex. Otherwise, a paused task could check them
+    //       (and find that it is still paused), we could then update them and notify our pause condition variable, and
+    //       only then would the task start waiting on our pause condition variable, i.e. it would never be woken up.
+
+    {
+        const std::scoped_lock<std::mutex> pauseLock(mPauseMutex);
+
+        mRunControl.fetch_and(~INSTANCE_RUN_CONTROL_PAUSE, std::memory_order_relaxed);
+    }
 
     mPauseConditionVariable.notify_all();
 }
 
 void SedInstance::Impl::stopRun()
 {
-    mRunControl.fetch_or(INSTANCE_RUN_CONTROL_STOP, std::memory_order_relaxed);
+    // Note: see the note in resumeRun().
+
+    {
+        const std::scoped_lock<std::mutex> pauseLock(mPauseMutex);
+
+        mRunControl.fetch_or(INSTANCE_RUN_CONTROL_STOP, std::memory_order_relaxed);
+    }
 
     mPauseConditionVariable.notify_all();
 }
@@ -313,13 +333,16 @@ SedInstance::SedInstance(const SedDocumentPtr &pDocument)
 SedInstance::~SedInstance()
 {
     // 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.
+    // Note #1: we stop any run before waiting for it since a paused run would otherwise never complete, i.e. we would
+    //          wait for it forever.
+    // Note #2: 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()->stopRun();
         pimpl()->waitForRun();
 #ifndef CODE_COVERAGE_ENABLED
     } catch (...) { // NOLINT(bugprone-empty-catch)
@@ -345,6 +368,10 @@ SedInstance::Status SedInstance::status() const noexcept
 
 double SedInstance::run()
 {
+    // Reset our control flags (see the note in SedInstance::Impl::run()).
+
+    pimpl()->mRunControl.store(INSTANCE_RUN_CONTROL_NONE, std::memory_order_relaxed);
+
     return pimpl()->run();
 }
 

tests/api/sed/instancetests.cpp

@@ -264,6 +264,79 @@ TEST(InstanceSedTest, stopRunWhenNotRunning)
     EXPECT_DOUBLE_EQ(instance->progress(), 0.0);
 }
 
+TEST(InstanceSedTest, stopRunWhenNotRunningDoesNotAffectNextRun)
+{
+    auto file {libOpenCOR::File::create(libOpenCOR::resourcePath("cellml_2.cellml"))};
+    auto document {libOpenCOR::SedDocument::create(file)};
+    auto instance {document->instantiate()};
+
+    instance->stopRun();
+    instance->run();
+
+    EXPECT_DOUBLE_EQ(instance->progress(), 1.0);
+    EXPECT_FALSE(instance->hasIssues());
+
+    instance->stopRun();
+
+    EXPECT_TRUE(instance->startRun());
+    EXPECT_GT(instance->waitForRun(), 0.0);
+    EXPECT_DOUBLE_EQ(instance->progress(), 1.0);
+    EXPECT_FALSE(instance->hasIssues());
+}
+
+TEST(InstanceSedTest, stopRunRightAfterStartRun)
+{
+    static const auto SIMULATION_PROPERTY {1000000};
+
+    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(SIMULATION_PROPERTY);
+    simulation->setOutputEndTime(static_cast<double>(SIMULATION_PROPERTY));
+
+    auto instance {document->instantiate()};
+
+    EXPECT_TRUE(instance->startRun());
+
+    instance->stopRun();
+    instance->waitForRun();
+
+    EXPECT_EQ(instance->status(), libOpenCOR::SedInstance::Status::IDLE);
+    EXPECT_LT(instance->progress(), 1.0);
+    EXPECT_FALSE(instance->hasIssues());
+}
+
+TEST(InstanceSedTest, pauseRunRightAfterStartRun)
+{
+    static const auto SIMULATION_PROPERTY {1000000};
+    static const auto PAUSE_SLEEP = 50;
+
+    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(SIMULATION_PROPERTY);
+    simulation->setOutputEndTime(static_cast<double>(SIMULATION_PROPERTY));
+
+    auto instance {document->instantiate()};
+
+    EXPECT_TRUE(instance->startRun());
+
+    instance->pauseRun();
+
+    std::this_thread::sleep_for(std::chrono::milliseconds(PAUSE_SLEEP));
+
+    EXPECT_EQ(instance->status(), libOpenCOR::SedInstance::Status::PAUSED);
+
+    instance->stopRun();
+    instance->waitForRun();
+
+    EXPECT_EQ(instance->status(), libOpenCOR::SedInstance::Status::IDLE);
+    EXPECT_LT(instance->progress(), 1.0);
+    EXPECT_FALSE(instance->hasIssues());
+}
+
 TEST(InstanceSedTest, stopRunResultsHaveNans)
 {
     static const auto SIMULATION_PROPERTY {1000000};
@@ -428,6 +501,42 @@ TEST(InstanceSedTest, pauseRunThenStopRun)
     EXPECT_FALSE(instance->hasIssues());
 }
 
+TEST(InstanceSedTest, deletePausedInstance)
+{
+    static const auto SIMULATION_PROPERTY {1000000};
+    static const auto WAIT_ITERATIONS = 60000;
+
+    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(SIMULATION_PROPERTY);
+    simulation->setOutputEndTime(static_cast<double>(SIMULATION_PROPERTY));
+
+    auto instance {document->instantiate()};
+    const auto instanceTask {instance->tasks()[0]};
+
+    EXPECT_TRUE(instance->startRun());
+
+    for (size_t i {0}; i < WAIT_ITERATIONS; ++i) {
+        if (instance->progress() > 0.0) {
+            break;
+        }
+
+        std::this_thread::sleep_for(std::chrono::milliseconds(1));
+    }
+
+    instance->pauseRun();
+
+    EXPECT_EQ(instance->status(), libOpenCOR::SedInstance::Status::PAUSED);
+
+    // Delete our paused instance, something that would hang if our run was not stopped first.
+
+    instance.reset();
+
+    EXPECT_LT(instanceTask->progress(), 1.0);
+}
+
 TEST(InstanceSedTest, pauseRunAndResumeRunWithNaturalCompletion)
 {
     static const auto MODERATE_STEP_COUNT {50000};

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

@@ -373,6 +373,85 @@ test.describe('Sed instance tests', () => {
     assert.strictEqual(instance.progress, 0.0);
   });
 
+  test('Stop run when not running does not affect next run', () => {
+    const file = new loc.File(utils.resourcePath('cellml_2.cellml'));
+
+    file.setContents(utils.fileContents(file.path));
+
+    const document = new loc.SedDocument(file);
+    const instance = document.instantiate();
+
+    instance.stopRun();
+    instance.run();
+
+    assert.strictEqual(instance.progress, 1.0);
+    assert.strictEqual(instance.hasIssues, false);
+
+    instance.stopRun();
+
+    assert.strictEqual(instance.startRun(), true);
+    assert.ok(instance.waitForRun() > 0.0);
+    assert.strictEqual(instance.progress, 1.0);
+    assert.strictEqual(instance.hasIssues, false);
+  });
+
+  test('Stop run right after start run', () => {
+    const SIMULATION_PROPERTY = 1000000;
+
+    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 = SIMULATION_PROPERTY;
+    simulation.outputEndTime = SIMULATION_PROPERTY;
+
+    const instance = document.instantiate();
+
+    assert.strictEqual(instance.startRun(), true);
+
+    instance.stopRun();
+    instance.waitForRun();
+
+    assert.strictEqual(instance.status, loc.SedInstance.Status.IDLE);
+    assert.ok(instance.progress < 1.0);
+    assert.strictEqual(instance.hasIssues, false);
+  });
+
+  test('Pause run right after start run', async () => {
+    const SIMULATION_PROPERTY = 1000000;
+    const PAUSE_SLEEP = 50;
+
+    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 = SIMULATION_PROPERTY;
+    simulation.outputEndTime = SIMULATION_PROPERTY;
+
+    const instance = document.instantiate();
+
+    assert.strictEqual(instance.startRun(), true);
+
+    instance.pauseRun();
+
+    await sleep(PAUSE_SLEEP);
+
+    assert.strictEqual(instance.status, loc.SedInstance.Status.PAUSED);
+
+    instance.stopRun();
+    instance.waitForRun();
+
+    assert.strictEqual(instance.status, loc.SedInstance.Status.IDLE);
+    assert.ok(instance.progress < 1.0);
+    assert.strictEqual(instance.hasIssues, false);
+  });
+
   test('Pause run and resume run', async () => {
     const SIMULATION_PROPERTY = 1000000;
     const WAIT_ITERATIONS = 60000;
@@ -479,6 +558,44 @@ test.describe('Sed instance tests', () => {
     assert.strictEqual(instance.hasIssues, false);
   });
 
+  test('Delete paused instance', async () => {
+    const SIMULATION_PROPERTY = 1000000;
+    const WAIT_ITERATIONS = 60000;
+
+    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 = SIMULATION_PROPERTY;
+    simulation.outputEndTime = SIMULATION_PROPERTY;
+
+    const instance = document.instantiate();
+    const instanceTask = instance.tasks.get(0);
+
+    assert.strictEqual(instance.startRun(), true);
+
+    for (let i = 0; i < WAIT_ITERATIONS; ++i) {
+      if (instance.progress > 0.0) {
+        break;
+      }
+
+      await sleep(1);
+    }
+
+    instance.pauseRun();
+
+    assert.strictEqual(instance.status, loc.SedInstance.Status.PAUSED);
+
+    // Delete our paused instance, something that would hang if our run was not stopped first.
+
+    instance.delete();
+
+    assert.ok(instanceTask.progress < 1.0);
+  });
+
   test('Pause run and resume run with natural completion', async () => {
     const moderateStepCount = 50000;
     const WAIT_ITERATIONS = 60000;