Unverified Commit 8f664ba4 authored by Gigg, Martyn Anthony's avatar Gigg, Martyn Anthony Committed by GitHub
Browse files

Merge pull request #28488 from mantidproject/Fix_lifetime_issue_with_algorithms

Add a short delay before algorithm garbage collection
parents 94804a7f 94adede8
......@@ -12,6 +12,7 @@
#include "MantidAPI/DllConfig.h"
#include "MantidAPI/IAlgorithm.h"
#include "MantidAPI/IndexTypeProperty.h"
#include "MantidKernel/DateAndTime.h"
#include "MantidKernel/IValidator.h"
#include "MantidKernel/PropertyManagerOwner.h"
......@@ -224,6 +225,7 @@ public:
bool isInitialized() const override;
bool isExecuted() const override;
bool isRunning() const override;
bool isReadyForGarbageCollection() const override;
using Kernel::PropertyManagerOwner::getProperty;
......@@ -502,6 +504,9 @@ private:
/// (MPI) communicator used when executing the algorithm.
std::unique_ptr<Parallel::Communicator> m_communicator;
/// The earliest this class should be considered for garbage collection
Mantid::Types::Core::DateAndTime m_gcTime;
};
/// Typedef for a shared pointer to an Algorithm
......
......@@ -132,6 +132,9 @@ public:
/// True if the algorithm is running.
virtual bool isRunning() const = 0;
/// True if the algorithm is ready for garbage collection.
virtual bool isReadyForGarbageCollection() const = 0;
/// To query whether algorithm is a child. Default to false
virtual bool isChild() const = 0;
......
......@@ -16,7 +16,6 @@
#include "MantidKernel/CompositeValidator.h"
#include "MantidKernel/ConfigService.h"
#include "MantidKernel/DateAndTime.h"
#include "MantidKernel/EmptyValues.h"
#include "MantidKernel/MultiThreaded.h"
#include "MantidKernel/PropertyWithValue.h"
......@@ -50,6 +49,10 @@ namespace {
/// Separator for workspace types in workspaceMethodOnTypes member
const std::string WORKSPACE_TYPES_SEPARATOR = ";";
/// The minimum number of seconds after execution that the algorithm should be
/// kept alive before garbage collection
const size_t DELAY_BEFORE_GC = 5;
class WorkspacePropertyValueIs {
public:
explicit WorkspacePropertyValueIs(const std::string &value)
......@@ -195,6 +198,15 @@ bool Algorithm::isRunning() const {
return (executionState() == ExecutionState::Running);
}
/// True if the algorithm is ready for garbage collection.
bool Algorithm::isReadyForGarbageCollection() const {
if ((executionState() == ExecutionState::Finished) &&
(Mantid::Types::Core::DateAndTime::getCurrentTime() > m_gcTime)) {
return true;
}
return false;
}
//---------------------------------------------------------------------------------------------
/** Add an observer to a notification
@param observer :: Reference to the observer to add
......@@ -674,7 +686,11 @@ bool Algorithm::executeInternal() {
" seconds\n");
reportCompleted(duration);
} catch (std::runtime_error &ex) {
m_gcTime = Mantid::Types::Core::DateAndTime::getCurrentTime() +=
(Mantid::Types::Core::DateAndTime::ONE_SECOND * DELAY_BEFORE_GC);
setResultState(ResultState::Failed);
notificationCenter().postNotification(
new ErrorNotification(this, ex.what()));
this->unlockWorkspaces();
if (m_isChildAlgorithm || m_runningAsync || m_rethrow)
throw;
......@@ -683,11 +699,15 @@ bool Algorithm::executeInternal() {
<< "Error in execution of algorithm " << this->name() << '\n'
<< ex.what() << '\n';
}
} catch (std::logic_error &ex) {
m_gcTime = Mantid::Types::Core::DateAndTime::getCurrentTime() +=
(Mantid::Types::Core::DateAndTime::ONE_SECOND * DELAY_BEFORE_GC);
notificationCenter().postNotification(
new ErrorNotification(this, ex.what()));
} catch (std::logic_error &ex) {
setResultState(ResultState::Failed);
this->unlockWorkspaces();
if (m_isChildAlgorithm || m_runningAsync || m_rethrow)
throw;
else {
......@@ -695,37 +715,41 @@ bool Algorithm::executeInternal() {
<< "Logic Error in execution of algorithm " << this->name() << '\n'
<< ex.what() << '\n';
}
notificationCenter().postNotification(
new ErrorNotification(this, ex.what()));
}
} catch (CancelException &ex) {
setResultState(ResultState::Failed);
m_runningAsync = false;
getLogger().warning() << this->name() << ": Execution cancelled by user.\n";
m_gcTime = Mantid::Types::Core::DateAndTime::getCurrentTime() +=
(Mantid::Types::Core::DateAndTime::ONE_SECOND * DELAY_BEFORE_GC);
setResultState(ResultState::Failed);
notificationCenter().postNotification(
new ErrorNotification(this, ex.what()));
this->unlockWorkspaces();
throw;
}
// Gaudi also specifically catches GaudiException & std:exception.
catch (std::exception &ex) {
setResultState(ResultState::Failed);
m_runningAsync = false;
notificationCenter().postNotification(
new ErrorNotification(this, ex.what()));
getLogger().error() << "Error in execution of algorithm " << this->name()
<< ":\n"
<< ex.what() << "\n";
m_gcTime = Mantid::Types::Core::DateAndTime::getCurrentTime() +=
(Mantid::Types::Core::DateAndTime::ONE_SECOND * DELAY_BEFORE_GC);
setResultState(ResultState::Failed);
notificationCenter().postNotification(
new ErrorNotification(this, ex.what()));
this->unlockWorkspaces();
throw;
}
catch (...) {
// Execution failed with an unknown exception object
setResultState(ResultState::Failed);
m_runningAsync = false;
m_gcTime = Mantid::Types::Core::DateAndTime::getCurrentTime() +=
(Mantid::Types::Core::DateAndTime::ONE_SECOND * DELAY_BEFORE_GC);
setResultState(ResultState::Failed);
notificationCenter().postNotification(
new ErrorNotification(this, "UNKNOWN Exception is caught in exec()"));
getLogger().error() << this->name()
......@@ -737,13 +761,16 @@ bool Algorithm::executeInternal() {
// Unlock the locked workspaces
this->unlockWorkspaces();
// Only gets to here if algorithm ended normally
m_gcTime = Mantid::Types::Core::DateAndTime::getCurrentTime() +=
(Mantid::Types::Core::DateAndTime::ONE_SECOND * DELAY_BEFORE_GC);
if (algIsExecuted) {
setResultState(ResultState::Success);
}
// Only gets to here if algorithm ended normally
notificationCenter().postNotification(
new FinishedNotification(this, algIsExecuted));
return algIsExecuted;
new FinishedNotification(this, isExecuted()));
return isExecuted();
}
//---------------------------------------------------------------------------------------------
......
......@@ -190,11 +190,11 @@ void AlgorithmManagerImpl::cancelAll() {
/// place
size_t AlgorithmManagerImpl::removeFinishedAlgorithms() {
std::vector<IAlgorithm_const_sptr> theCompletedInstances;
std::copy_if(
m_managed_algs.cbegin(), m_managed_algs.cend(),
std::back_inserter(theCompletedInstances), [](const auto &algorithm) {
return (algorithm->executionState() == ExecutionState::Finished);
});
std::copy_if(m_managed_algs.cbegin(), m_managed_algs.cend(),
std::back_inserter(theCompletedInstances),
[](const auto &algorithm) {
return (algorithm->isReadyForGarbageCollection());
});
for (auto completedAlg : theCompletedInstances) {
auto itend = m_managed_algs.end();
for (auto it = m_managed_algs.begin(); it != itend; ++it) {
......
......@@ -1083,6 +1083,7 @@ add_subdirectory(test)
# Auto-generate exports header
target_include_directories(Algorithms PUBLIC ${CMAKE_BINARY_DIR}/Framework/Algorithms)
target_include_directories(Algorithms PUBLIC ${CMAKE_BINARY_DIR}/Framework/Types)
generate_mantid_export_header(Algorithms FALSE)
# Installation settings
......
......@@ -12,7 +12,8 @@
#include <set>
#include <string>
namespace Mantid::Kernel {
namespace Mantid {
namespace Kernel {
class MANTID_KERNEL_DLL NexusHDF5Descriptor {
......@@ -90,4 +91,5 @@ private:
const std::map<std::string, std::set<std::string>> m_allEntries;
};
} // namespace Mantid::Kernel
} // namespace Kernel
} // namespace Mantid
......@@ -24,6 +24,8 @@ if(CXXTEST_FOUND)
set_target_properties(${_pythoninterface_test_target_name}
PROPERTIES COMPILE_FLAGS "/w44244")
endif()
target_include_directories(${_pythoninterface_test_target_name} PUBLIC ${CMAKE_BINARY_DIR}/Framework/Types)
target_link_libraries(${_pythoninterface_test_target_name}
LINK_PRIVATE
${TCMALLOC_LIBRARIES_LINKTIME}
......
......@@ -4,6 +4,7 @@ if(CXXTEST_FOUND)
)
cxxtest_add_test(RemoteAlgorithmsTest ${TEST_FILES})
target_include_directories(RemoteAlgorithmsTest PUBLIC ${CMAKE_BINARY_DIR}/Framework/Types)
target_link_libraries(RemoteAlgorithmsTest
LINK_PRIVATE
${TCMALLOC_LIBRARIES_LINKTIME}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment