diff --git a/VERSION.txt b/VERSION.txt index c88490b8f..3473d135a 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.20261009.2 +1.20261009.3 diff --git a/src/api/libopencor/sedinstance.h b/src/api/libopencor/sedinstance.h index bed4af382..ae581bc9b 100644 --- a/src/api/libopencor/sedinstance.h +++ b/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(); diff --git a/src/sed/sedinstance.cpp b/src/sed/sedinstance.cpp index 6ea4cdae2..1306a617d 100644 --- a/src/sed/sedinstance.cpp +++ b/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 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 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(); } diff --git a/tests/api/sed/instancetests.cpp b/tests/api/sed/instancetests.cpp index 1fa222d85..52ea76784 100644 --- a/tests/api/sed/instancetests.cpp +++ b/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(document->simulations()[0])}; + + simulation->setNumberOfSteps(SIMULATION_PROPERTY); + simulation->setOutputEndTime(static_cast(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(document->simulations()[0])}; + + simulation->setNumberOfSteps(SIMULATION_PROPERTY); + simulation->setOutputEndTime(static_cast(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(document->simulations()[0])}; + + simulation->setNumberOfSteps(SIMULATION_PROPERTY); + simulation->setOutputEndTime(static_cast(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}; diff --git a/tests/bindings/javascript/sed.instance.test.js b/tests/bindings/javascript/sed.instance.test.js index 943dab88d..d8a4dd7ed 100644 --- a/tests/bindings/javascript/sed.instance.test.js +++ b/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; diff --git a/tests/bindings/python/test_sed_instance.py b/tests/bindings/python/test_sed_instance.py index b8f6cf840..a9e3e926c 100644 --- a/tests/bindings/python/test_sed_instance.py +++ b/tests/bindings/python/test_sed_instance.py @@ -333,6 +333,74 @@ def test_stop_run_when_not_running(): assert instance.progress == 0.0 +def test_stop_run_when_not_running_does_not_affect_next_run(): + file = loc.File(utils.resource_path("cellml_2.cellml")) + document = loc.SedDocument(file) + instance = document.instantiate() + + instance.stop_run() + instance.run() + + assert instance.progress == 1.0 + assert not instance.has_issues + + instance.stop_run() + + assert instance.start_run() is True + assert instance.wait_for_run() > 0.0 + assert instance.progress == 1.0 + assert not instance.has_issues + + +def test_stop_run_right_after_start_run(): + SIMULATION_PROPERTY = 1000000 + + file = loc.File(utils.resource_path("cellml_2.cellml")) + document = loc.SedDocument(file) + simulation = document.simulations[0] + simulation.number_of_steps = SIMULATION_PROPERTY + simulation.output_end_time = float(SIMULATION_PROPERTY) + + instance = document.instantiate() + + assert instance.start_run() is True + + instance.stop_run() + instance.wait_for_run() + + assert instance.status == loc.SedInstance.Status.Idle + assert instance.progress < 1.0 + assert not instance.has_issues + + +def test_pause_run_right_after_start_run(): + SIMULATION_PROPERTY = 1000000 + PAUSE_SLEEP = 0.05 + + file = loc.File(utils.resource_path("cellml_2.cellml")) + document = loc.SedDocument(file) + simulation = document.simulations[0] + simulation.number_of_steps = SIMULATION_PROPERTY + simulation.output_end_time = float(SIMULATION_PROPERTY) + + instance = document.instantiate() + + assert instance.start_run() is True + + instance.pause_run() + + time.sleep(PAUSE_SLEEP) + + assert instance.status == loc.SedInstance.Status.Paused + + instance.stop_run() + instance.wait_for_run() + + assert instance.status == loc.SedInstance.Status.Idle + assert instance.progress < 1.0 + assert not instance.has_issues + + def test_pause_run_and_resume_run(): SIMULATION_PROPERTY = 1000000 WAIT_ITERATIONS = 60000 @@ -422,6 +490,38 @@ def test_pause_run_then_stop_run(): assert not instance.has_issues +def test_delete_paused_instance(): + SIMULATION_PROPERTY = 1000000 + WAIT_ITERATIONS = 60000 + + file = loc.File(utils.resource_path("cellml_2.cellml")) + document = loc.SedDocument(file) + simulation = document.simulations[0] + simulation.number_of_steps = SIMULATION_PROPERTY + simulation.output_end_time = float(SIMULATION_PROPERTY) + + instance = document.instantiate() + instance_task = instance.tasks[0] + + assert instance.start_run() is True + + for _ in range(WAIT_ITERATIONS): + if instance.progress > 0.0: + break + + time.sleep(0.001) + + instance.pause_run() + + assert instance.status == loc.SedInstance.Status.Paused + + # Delete our paused instance, something that would hang if our run was not stopped first. + + del instance + + assert instance_task.progress < 1.0 + + def test_pause_run_and_resume_run_with_natural_completion(): moderate_step_count = 50000 WAIT_ITERATIONS = 60000