Merge pull request #28272 from mantidproject/28093_only_retain_running_algorithms_2

Change Algorithm State tracking

Framework/API/inc/MantidAPI/Algorithm.h

@@ -219,6 +219,11 @@ public:
   bool execute() override final;
   void executeAsChildAlg() override;
   std::map<std::string, std::string> validateInputs() override;
+
+  /// Gets the current execution state
+  ExecutionState executionState() const override;
+  /// Gets the current result State
+  ResultState resultState() const override;
   bool isInitialized() const override;
   bool isExecuted() const override;
   bool isRunning() const override;
@@ -330,8 +335,10 @@ protected:
   friend class AlgorithmProxy;
   void initializeFromProxy(const AlgorithmProxy &);
 
-  void setInitialized();
-  void setExecuted(bool state);
+  void setExecutionState(
+      const ExecutionState state); ///< Sets the current execution state
+  void
+  setResultState(const ResultState state); ///< Sets the result execution state
 
   void store();
 
@@ -454,14 +461,13 @@ private:
   mutable std::unique_ptr<Poco::NObserver<Algorithm, ProgressNotification>>
       m_progressObserver;
 
-  bool m_isInitialized;         ///< Algorithm has been initialized flag
-  bool m_isExecuted;            ///< Algorithm is executed flag
+  std::atomic<ExecutionState> m_executionState; ///< the current execution state
+  std::atomic<ResultState> m_resultState;       ///< the current result State
   bool m_isChildAlgorithm;      ///< Algorithm is a child algorithm
   bool m_recordHistoryForChild; ///< Flag to indicate whether history should be
                                 /// recorded. Applicable to child algs only
   bool m_alwaysStoreInADS; ///< Always store in the ADS, even for child algos
   bool m_runningAsync;     ///< Algorithm is running asynchronously
-  std::atomic<bool> m_running; ///< Algorithm is running
   bool m_rethrow; ///< Algorithm should rethrow exceptions while executing
   bool m_isAlgStartupLoggingEnabled; /// Whether to log alg startup and
                                      /// closedown messages from the base class

Framework/API/inc/MantidAPI/AlgorithmProxy.h

@@ -88,6 +88,12 @@ public:
     throw std::runtime_error("Not implemented.");
   }
   Poco::ActiveResult<bool> executeAsync() override;
+
+  /// Gets the current execution state
+  ExecutionState executionState() const override;
+  /// Gets the current result State
+  ResultState resultState() const override;
+
   bool isInitialized() const override;
   bool isExecuted() const override;
 
@@ -179,11 +185,13 @@ private:
   const int m_version;         ///< version of the real algorithm
 
   mutable boost::shared_ptr<Algorithm>
-      m_alg;         ///< Shared pointer to a real algorithm. Created on demand
-  bool m_isExecuted; ///< Executed flag
+      m_alg; ///< Shared pointer to a real algorithm. Created on demand
+
+  ExecutionState m_executionState; ///< the current execution state
+  ResultState m_resultState;       ///< the current result State
   bool m_isLoggingEnabled; ///< is the logging of the underlying algorithm
-  /// enabled
-  int m_loggingOffset;               ///< the logging priority offset
+                           /// enabled
+  int m_loggingOffset;     ///< the logging priority offset
   bool m_isAlgStartupLoggingEnabled; /// Whether to log alg startup and
                                      /// closedown messages from the base class
                                      /// (default = true)

Framework/API/inc/MantidAPI/IAlgorithm.h

@@ -28,6 +28,11 @@ namespace API {
  */
 using AlgorithmID = void *;
 
+/// The current state of the algorithm object
+enum class ExecutionState { Uninitialized, Initialized, Running, Finished };
+/// The validity of the results of the algorithm object
+enum class ResultState { NotFinished, Failed, Success };
+
 /**
  IAlgorithm is the interface implemented by the Algorithm base class.
  Concrete algorithms, derived from the Algorithm base class are controlled
@@ -109,6 +114,12 @@ public:
   /// Execute as a Child Algorithm, with try/catch
   virtual void executeAsChildAlg() = 0;
 
+  /// Gets the current execution state
+  virtual ExecutionState executionState() const = 0;
+
+  /// Gets the currnet result State
+  virtual ResultState resultState() const = 0;
+
   /// Check whether the algorithm is initialized properly
   virtual bool isInitialized() const = 0;
   /// Check whether the algorithm has already been executed

Framework/API/src/Algorithm.cpp

@@ -98,9 +98,10 @@ Algorithm::Algorithm()
     : PropertyManagerOwner(), m_cancel(false), m_parallelException(false),
       m_log("Algorithm"), g_log(m_log), m_groupSize(0), m_executeAsync(nullptr),
       m_notificationCenter(nullptr), m_progressObserver(nullptr),
-      m_isInitialized(false), m_isExecuted(false), m_isChildAlgorithm(false),
+      m_executionState(ExecutionState::Uninitialized),
+      m_resultState(ResultState::NotFinished), m_isChildAlgorithm(false),
       m_recordHistoryForChild(false), m_alwaysStoreInADS(true),
-      m_runningAsync(false), m_running(false), m_rethrow(false),
+      m_runningAsync(false), m_rethrow(false),
       m_isAlgStartupLoggingEnabled(true), m_startChildProgress(0.),
       m_endChildProgress(0.), m_algorithmID(this), m_singleGroup(-1),
       m_groupsHaveSimilarNames(false), m_inputWorkspaceHistories(),
@@ -114,24 +115,37 @@ Algorithm::~Algorithm() {}
 //===================================
 //=============================================================================================
 
+/// Gets the current execution state
+ExecutionState Algorithm::executionState() const { return m_executionState; }
+
+/// Sets the current execution state
+void Algorithm::setExecutionState(const ExecutionState state) {
+  m_executionState = state;
+}
+
+/// Gets the current result State
+ResultState Algorithm::resultState() const { return m_resultState; }
+
+/// Sets the result execution state
+/// if set to Success or Failed will also set the execution state to finished
+void Algorithm::setResultState(const ResultState state) {
+  if (state != ResultState::NotFinished) {
+    setExecutionState(ExecutionState::Finished);
+  }
+  m_resultState = state;
+}
+
 //---------------------------------------------------------------------------------------------
 /// Has the Algorithm already been initialized
-bool Algorithm::isInitialized() const { return m_isInitialized; }
+bool Algorithm::isInitialized() const {
+  return (m_executionState != ExecutionState::Uninitialized);
+}
 
 /// Has the Algorithm already been executed
-bool Algorithm::isExecuted() const { return m_isExecuted; }
-
-//---------------------------------------------------------------------------------------------
-/// Set the Algorithm initialized state
-void Algorithm::setInitialized() { m_isInitialized = true; }
-
-/** Set the executed flag to the specified state
-// Public in Gaudi - don't know why and will leave here unless we find a reason
-otherwise
-//     Also don't know reason for different return type and argument.
-@param state :: New executed state
-*/
-void Algorithm::setExecuted(bool state) { m_isExecuted = state; }
+bool Algorithm::isExecuted() const {
+  return ((executionState() == ExecutionState::Finished) &&
+          (resultState() == ResultState::Success));
+}
 
 //---------------------------------------------------------------------------------------------
 /** To query whether algorithm is a child.
@@ -178,7 +192,9 @@ bool Algorithm::getAlwaysStoreInADS() const { return m_alwaysStoreInADS; }
 void Algorithm::setRethrows(const bool rethrow) { this->m_rethrow = rethrow; }
 
 /// True if the algorithm is running.
-bool Algorithm::isRunning() const { return m_running; }
+bool Algorithm::isRunning() const {
+  return (executionState() == ExecutionState::Running);
+}
 
 //---------------------------------------------------------------------------------------------
 /**  Add an observer to a notification
@@ -262,7 +278,7 @@ const std::string Algorithm::workspaceMethodInputProperty() const { return ""; }
  */
 void Algorithm::initialize() {
   // Bypass the initialization if the algorithm has already been initialized.
-  if (m_isInitialized)
+  if (isInitialized())
     return;
 
   g_log.setName(this->name());
@@ -277,7 +293,7 @@ void Algorithm::initialize() {
 
     // Indicate that this Algorithm has been initialized to prevent duplicate
     // attempts.
-    setInitialized();
+    setExecutionState(ExecutionState::Initialized);
   } catch (std::runtime_error &) {
     throw;
   }
@@ -490,6 +506,7 @@ void Algorithm::unlockWorkspaces() {
 
 bool Algorithm::executeInternal() {
   Timer timer;
+  bool algIsExecuted = false;
   AlgorithmManager::Instance().notifyAlgorithmStarting(this->getAlgorithmID());
   {
     auto *depo = dynamic_cast<DeprecatedAlgorithm *>(this);
@@ -548,7 +565,7 @@ bool Algorithm::executeInternal() {
                         << ex.what() << "\n";
     notificationCenter().postNotification(
         new ErrorNotification(this, ex.what()));
-    m_running = false;
+    setResultState(ResultState::Failed);
     if (m_isChildAlgorithm || m_runningAsync || m_rethrow) {
       m_runningAsync = false;
       throw;
@@ -618,10 +635,7 @@ bool Algorithm::executeInternal() {
   // Invoke exec() method of derived class and catch all uncaught exceptions
   try {
     try {
-      setExecuted(false);
-      if (!isChild()) {
-        m_running = true;
-      }
+      setExecutionState(ExecutionState::Running);
 
       startTime = Mantid::Types::Core::DateAndTime::getCurrentTime();
       // Call the concrete algorithm's exec method
@@ -646,7 +660,9 @@ bool Algorithm::executeInternal() {
       if (m_alwaysStoreInADS)
         this->store();
 
-      setExecuted(true);
+      // just cache the value internally, it is set at the very end of this
+      // method
+      algIsExecuted = true;
 
       // Log that execution has completed.
       getLogger().debug(
@@ -659,6 +675,7 @@ bool Algorithm::executeInternal() {
           " seconds\n");
       reportCompleted(duration);
     } catch (std::runtime_error &ex) {
+      setResultState(ResultState::Failed);
       this->unlockWorkspaces();
       if (m_isChildAlgorithm || m_runningAsync || m_rethrow)
         throw;
@@ -669,8 +686,8 @@ bool Algorithm::executeInternal() {
       }
       notificationCenter().postNotification(
           new ErrorNotification(this, ex.what()));
-      m_running = false;
     } catch (std::logic_error &ex) {
+      setResultState(ResultState::Failed);
       this->unlockWorkspaces();
       if (m_isChildAlgorithm || m_runningAsync || m_rethrow)
         throw;
@@ -681,11 +698,10 @@ bool Algorithm::executeInternal() {
       }
       notificationCenter().postNotification(
           new ErrorNotification(this, ex.what()));
-      m_running = false;
     }
   } catch (CancelException &ex) {
+    setResultState(ResultState::Failed);
     m_runningAsync = false;
-    m_running = false;
     getLogger().warning() << this->name() << ": Execution cancelled by user.\n";
     notificationCenter().postNotification(
         new ErrorNotification(this, ex.what()));
@@ -694,9 +710,8 @@ bool Algorithm::executeInternal() {
   }
   // Gaudi also specifically catches GaudiException & std:exception.
   catch (std::exception &ex) {
-    setExecuted(false);
+    setResultState(ResultState::Failed);
     m_runningAsync = false;
-    m_running = false;
 
     notificationCenter().postNotification(
         new ErrorNotification(this, ex.what()));
@@ -708,10 +723,9 @@ bool Algorithm::executeInternal() {
   }
 
   catch (...) {
-    // Execution failed
-    setExecuted(false);
+    // Execution failed with an unknown exception object
+    setResultState(ResultState::Failed);
     m_runningAsync = false;
-    m_running = false;
 
     notificationCenter().postNotification(
         new ErrorNotification(this, "UNKNOWN Exception is caught in exec()"));
@@ -724,10 +738,13 @@ bool Algorithm::executeInternal() {
   // Unlock the locked workspaces
   this->unlockWorkspaces();
 
-  notificationCenter().postNotification(
-      new FinishedNotification(this, isExecuted()));
   // Only gets to here if algorithm ended normally
-  return isExecuted();
+  if (algIsExecuted) {
+    setResultState(ResultState::Success);
+  }
+  notificationCenter().postNotification(
+      new FinishedNotification(this, algIsExecuted));
+  return algIsExecuted;
 }
 
 //---------------------------------------------------------------------------------------------
@@ -1275,16 +1292,14 @@ bool Algorithm::doCallProcessGroups(
     // but we also need to update flags in the parent algorithm and
     // send an ErrorNotification (because the child isn't registered with the
     // AlgorithmMonitor).
-    setExecuted(false);
+    setResultState(ResultState::Failed);
     m_runningAsync = false;
-    m_running = false;
     notificationCenter().postNotification(
         new ErrorNotification(this, ex.what()));
     throw;
   } catch (...) {
-    setExecuted(false);
+    setResultState(ResultState::Failed);
     m_runningAsync = false;
-    m_running = false;
     notificationCenter().postNotification(new ErrorNotification(
         this, "UNKNOWN Exception caught from processGroups"));
     throw;
@@ -1313,9 +1328,11 @@ bool Algorithm::doCallProcessGroups(
 
     // Log that execution has completed.
     reportCompleted(duration, true /* this is for group processing*/);
+    setResultState(ResultState::Success);
+  } else {
+    setResultState(ResultState::Failed);
   }
 
-  setExecuted(completed);
   notificationCenter().postNotification(
       new FinishedNotification(this, isExecuted()));
 
@@ -1763,7 +1780,7 @@ void Algorithm::reportCompleted(const double &duration,
     getLogger().debug() << name() << " finished with isChild = " << isChild()
                         << '\n';
   }
-  m_running = false;
+  setExecutionState(ExecutionState::Finished);
 }
 
 /** Registers the usage of the algorithm with the UsageService

Framework/API/src/AlgorithmProxy.cpp

@@ -28,10 +28,11 @@ AlgorithmProxy::AlgorithmProxy(Algorithm_sptr alg)
       m_name(alg->name()), m_category(alg->category()),
       m_categorySeparator(alg->categorySeparator()), m_seeAlso(alg->seeAlso()),
       m_alias(alg->alias()), m_summary(alg->summary()),
-      m_version(alg->version()), m_alg(alg), m_isExecuted(),
-      m_isLoggingEnabled(true), m_loggingOffset(0),
-      m_isAlgStartupLoggingEnabled(true), m_rethrow(false), m_isChild(false),
-      m_setAlwaysStoreInADS(true) {
+      m_version(alg->version()), m_alg(alg),
+      m_executionState(ExecutionState::Initialized),
+      m_resultState(ResultState::NotFinished), m_isLoggingEnabled(true),
+      m_loggingOffset(0), m_isAlgStartupLoggingEnabled(true), m_rethrow(false),
+      m_isChild(false), m_setAlwaysStoreInADS(true) {
   if (!alg) {
     throw std::logic_error("Unable to create a proxy algorithm.");
   }
@@ -78,7 +79,7 @@ bool AlgorithmProxy::execute() {
     throw;
   }
   stopped();
-  return m_isExecuted;
+  return isExecuted();
 }
 
 /** Asynchronous execution of the algorithm.
@@ -108,21 +109,31 @@ bool AlgorithmProxy::executeAsyncImpl(const Poco::Void &dummy) {
     throw;
   }
   stopped();
-  return m_isExecuted;
+  return isExecuted();
 }
 
+/// Gets the current execution state
+ExecutionState AlgorithmProxy::executionState() const {
+  return m_executionState;
+}
+
+/// Gets the current result State
+ResultState AlgorithmProxy::resultState() const { return m_resultState; }
+
 /// True if the algorithm is running.
 bool AlgorithmProxy::isRunning() const {
-  return m_alg ? m_alg->isRunning() : false;
+  return m_alg ? (m_alg->executionState() == ExecutionState::Running) : false;
 }
 
 /// Has the AlgorithmProxy already been initialized
 bool AlgorithmProxy::isInitialized() const {
-  return true; //!!!!!!!!!
+  return true; // Algorithm Proxies will always initialize the algorithm
 }
 
-/// Has the AlgorithmProxy already been executed
-bool AlgorithmProxy::isExecuted() const { return m_isExecuted; }
+/// Has the AlgorithmProxy already been executed successfully
+bool AlgorithmProxy::isExecuted() const {
+  return resultState() == ResultState::Success;
+}
 
 /// Cancel the execution of the algorithm
 void AlgorithmProxy::cancel() {
@@ -263,7 +274,8 @@ void AlgorithmProxy::createConcreteAlg(bool initOnly) {
 void AlgorithmProxy::stopped() {
   if (m_setAlwaysStoreInADS)
     dropWorkspaceReferences();
-  m_isExecuted = m_alg->isExecuted();
+  m_executionState = m_alg->executionState();
+  m_resultState = m_alg->resultState();
   m_alg.reset();
 }
 

Framework/API/test/AlgorithmManagerTest.h

@@ -82,8 +82,13 @@ public:
   int version() const override { return (1); }
   const std::string category() const override { return ("Cat1"); }
   const std::string summary() const override { return "Test summary"; }
-  // Override method so we can manipulate whether it appears to be running
-  bool isRunning() const override { return isRunningFlag; }
+  // Override methods so we can manipulate whether it appears to be running
+  ExecutionState executionState() const override {
+    return isRunningFlag ? ExecutionState::Running : ExecutionState::Finished;
+  }
+  ResultState resultState() const override {
+    return isRunningFlag ? ResultState::NotFinished : ResultState::Failed;
+  }
   void setIsRunningTo(bool runningFlag) { isRunningFlag = runningFlag; }
   void cancel() override { isRunningFlag = false; }
 };

Framework/API/test/AlgorithmProxyTest.h

@@ -152,6 +152,8 @@ public:
     alg->setProperty("prop2", 17);
     TS_ASSERT_THROWS_NOTHING(alg->execute());
     TS_ASSERT(alg->isExecuted());
+    TS_ASSERT_EQUALS(ExecutionState::Finished, alg->executionState());
+    TS_ASSERT_EQUALS(ResultState::Success, alg->resultState());
     int out = alg->getProperty("out");
     TS_ASSERT_EQUALS(out, 28);
   }

Framework/API/test/AlgorithmTest.h

@@ -291,11 +291,16 @@ public:
 
   void testExecute() {
     ToyAlgorithm myAlg;
+    TS_ASSERT_EQUALS(ExecutionState::Uninitialized, myAlg.executionState());
     TS_ASSERT_THROWS(myAlg.execute(), const std::runtime_error &);
     TS_ASSERT(!myAlg.isExecuted());
+    TS_ASSERT_EQUALS(ExecutionState::Uninitialized, myAlg.executionState());
     TS_ASSERT_THROWS_NOTHING(myAlg.initialize());
+    TS_ASSERT_EQUALS(ExecutionState::Initialized, myAlg.executionState());
     TS_ASSERT_THROWS_NOTHING(myAlg.execute());
     TS_ASSERT(myAlg.isExecuted());
+    TS_ASSERT_EQUALS(ExecutionState::Finished, myAlg.executionState());
+    TS_ASSERT_EQUALS(ResultState::Success, myAlg.resultState());
   }
 
   void testSetPropertyValue() {
@@ -332,11 +337,16 @@ public:
     alg.setProperty("PropertyA", 12);
     alg.setProperty("PropertyB", 5);
     TS_ASSERT_THROWS_ANYTHING(alg.execute());
+    // Algoritm never executed as property validation failed
     TS_ASSERT(!alg.isExecuted());
+    TS_ASSERT_EQUALS(ExecutionState::Initialized, alg.executionState());
+    TS_ASSERT_EQUALS(ResultState::NotFinished, alg.resultState());
 
     alg.setProperty("PropertyB", 15);
     TS_ASSERT_THROWS_NOTHING(alg.execute());
     TS_ASSERT(alg.isExecuted());
+    TS_ASSERT_EQUALS(ExecutionState::Finished, alg.executionState());
+    TS_ASSERT_EQUALS(ResultState::Success, alg.resultState());
   }
 
   void test_WorkspaceMethodFunctionsReturnEmptyByDefault() {