diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp index b668a2373..686b1f5a3 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp @@ -26,37 +26,46 @@ namespace score::mw::lifecycle::internal /// @brief A node finished activating successfully. struct [[nodiscard]] ActivationSuccessful { + /// @brief The identifier of the node that activated. IdentifierHash node_identifier; }; /// @brief A node failed to activate. struct [[nodiscard]] ActivationFailed { + /// @brief The identifier of the node that failed. IdentifierHash node_identifier; + /// @brief Description of the error. IComponent::ComponentError reason; }; /// @brief A node finished deactivating. struct [[nodiscard]] DeactivationComplete { + /// @brief The identifier of the node that deactivated. IdentifierHash node_identifier; }; /// @brief A node terminated without having been requested to. struct [[nodiscard]] UnexpectedTermination { + /// @brief The identifier of the node that terminated. IdentifierHash node_identifier; + /// @brief Description of the error. + IComponent::ComponentError reason; }; /// @brief A job was queued but cancelled by the time it was processed struct [[nodiscard]] JobSkipped { + /// @brief The identifier of the node that was skipped. IdentifierHash node_identifier; }; /// @brief Alive supervision has failed for the given process identifier. struct [[nodiscard]] SupervisionFailure { + /// @brief The identifier of the node that failed. IdentifierHash process_identifier; }; diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp index 2ae43239c..966a5f5dc 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp @@ -349,9 +349,7 @@ void Graph::handleComponentEvent(const ComponentEvent& event) } else if constexpr (std::is_same_v) { - // This is always an error after ready - an unexpected termination before ready is an activation failure - const auto error = IComponent::ComponentError::kErrorAfterReady; - abort(1, error); + abort(1, data.reason); // Need to clean up any leftover resources IComponent& failingComponent = componentOf(nodes_[data.node_identifier]); diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/graph.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/graph.hpp index 8abb965a1..3ddce5328 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/graph.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/graph.hpp @@ -172,14 +172,7 @@ class Graph final /// @brief Move assignment operator(deleted). Graph& operator=(Graph&&) noexcept = delete; - /// @brief Applies a ComponentEvent — produced by ProcessMonitor from worker/OS-handler thread - /// callbacks and drained on the main thread — to this graph. - /// @details Dispatches on the event's variant: - /// - ActivationSuccessful / DeactivationComplete: `nodeExecuted(node_identifier, {})` - /// - ActivationFailed: `nodeExecuted(node_identifier, make_unexpected(reason))` - /// - UnexpectedTermination: `abort(1, kErrorAfterReady)` — ProcessMonitor::terminated() only - /// pushes this event once a process has already reached its ready condition, so it is - /// always a post-ready crash. + /// @brief Applies a ComponentEvent to this graph. /// @param event The event to process. void handleComponentEvent(const ComponentEvent& event); diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp index f1ddd08b1..43602b740 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp @@ -543,7 +543,8 @@ TEST_F(GraphHandleComponentEventTest, unexpectedTerminationDuringSuccess) }), Return(osal::OsalReturnType::kSuccess))); - graph_->handleComponentEvent(UnexpectedTermination{component->getIdentifier()}); + graph_->handleComponentEvent( + UnexpectedTermination{component->getIdentifier(), IComponent::ComponentError::kErrorAfterReady}); EXPECT_EQ(graph_->getState(), GraphState::kUndefinedState); } @@ -571,7 +572,7 @@ TEST_F(GraphHandleComponentEventTest, unexpectedTerminationDuringTransition) Return(osal::OsalReturnType::kSuccess))); // The active component then crashes - graph_->handleComponentEvent(UnexpectedTermination{component_index}); + graph_->handleComponentEvent(UnexpectedTermination{component_index, IComponent::ComponentError::kErrorAfterReady}); const auto second_job = job_queue_->pop(); executeJobSuccessfully(second_job->value()); @@ -580,6 +581,33 @@ TEST_F(GraphHandleComponentEventTest, unexpectedTerminationDuringTransition) EXPECT_EQ(graph_->getPendingEvent(), ControlClientCode::kFailedUnexpectedTermination); } +class GraphTransitionFailuresTest : public GraphTest +{ +}; + +TEST_F(GraphTransitionFailuresTest, UnusualOrderOfFailures) +{ + RecordProperty( + "Description", + "Test that even if an unexpected termination is recieved before a successful activation, the graph reacts " + "correctly"); + + graph_->startTransition(IdentifierHash{run_target_name(0)}); + + const auto first_job = job_queue_->pop(); + const auto component_id = first_job.value()->component.get().getIdentifier(); + + EXPECT_CALL(process_interface_, requestTermination) + .WillOnce(Return(osal::OsalReturnType::kSuccess)); // Process is already gone, semaphore will time out + EXPECT_CALL(process_interface_, forceTermination).WillOnce(Return(osal::OsalReturnType::kFail)); + + graph_->handleComponentEvent(UnexpectedTermination{component_id, IComponent::ComponentError::kErrorAfterReady}); + graph_->handleComponentEvent(ActivationSuccessful{component_id}); + + EXPECT_EQ(graph_->getPendingEvent(), ControlClientCode::kFailedUnexpectedTermination); + EXPECT_EQ(graph_->getState(), GraphState::kUndefinedState) << "Graph should be in a final state"; +} + class GraphCancelTest : public GraphTest { }; diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index ab943feec..b901b4cc4 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -105,8 +105,7 @@ IComponent::RequestResult ProcessInfoNode::tryReportSuccess() std::optional ProcessInfoNode::getTimeForReport() const { - if (config_.component_properties.application_profile.application_type == - score::mw::lifecycle::internal::configuration::ApplicationType::Native) + if (!isReporting()) { return std::nullopt; } @@ -165,26 +164,37 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ LM_LOG_DEBUG() << "Process" << identifier_ << "( pid" << pid_ << ") terminated with exit code" << process_status; exit_code_ = process_status; IComponent::RequestResult res = {IComponent::RequestState::kWaiting}; - if (has_semaphore_.exchange(false)) + ProcessState starting = ProcessState::kStarting; + + if (config_.component_properties.application_profile.is_self_terminating && process_status == 0) + { + termination_result_ = TeminationResult::kOk; + } + else + { + termination_result_ = TeminationResult::kError; + } + + if (has_semaphore_.exchange(false)) // Termination was requested { - // Termination was requested, we don't care if exit code is not 0 (a SIGKILL will set exit code to 9) + // We don't care if the termination was valid, we requested it (e.g. a SIGKILL will set exit code to 9) setState(ProcessState::kTerminated); + unblockSync(); static_cast(terminator_.post()); } - else if (getState() < ProcessState::kRunning) + else if (process_state_.compare_exchange_strong(starting, ProcessState::kTerminated)) // Process still starting { - // Defer to the startup thread to handle this - setState(ProcessState::kTerminated); - + // In this case, we can't return anything because any definite result given here would invalidate startup + // recovery actions unblockSync(); } - else + else // This is a termination during normal execution { setState(ProcessState::kTerminated); - if (config_.component_properties.application_profile.is_self_terminating && process_status == 0) + + if (termination_result_ == TeminationResult::kOk) { - // Only valid case for a process to terminate without it being requested res = tryReportCompletion(ProcessState::kTerminated); } else @@ -204,6 +214,11 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ return res; } +bool ProcessInfoNode::isReporting() const +{ + return config_.component_properties.application_profile.application_type != configuration::ApplicationType::Native; +} + IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token stop_token) { LM_LOG_DEBUG() << "Starting process (" << identifier_ << ") from executable" << config_.deployment_config.bin_dir @@ -231,6 +246,7 @@ IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token s pid_ = 0; exit_code_ = 0; error = std::nullopt; + termination_result_ = TeminationResult::kNone; static_cast(setState(score::mw::lifecycle::ProcessState::kStarting)); // Cannot fail by design if (osal::OsalReturnType::kSuccess == process_handling_.process_interface_->startProcess(pid_, sync_, config_)) @@ -280,8 +296,42 @@ IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token s return tryReportError(error.value()); } - setState(ProcessState::kRunning); // Can fail if we've terminated already - return tryReportCompletion(ProcessState::kRunning); + if (setState(ProcessState::kRunning)) + { + return tryReportCompletion(ProcessState::kRunning); + } + + // We have terminated after starting up + SCORE_LANGUAGE_FUTURECPP_ASSERT_DBG_MESSAGE( + termination_result_ != TerminationResult::kNone, "setState(kRunning) failed without a termination result"); + // Assuming we have already waited for all ready conditions that require waiting, we have now started up and + // terminated. Therefore, as long as the termination was valid, we have satisfied any running or terminated + // condition. + if (termination_result_ == TeminationResult::kOk) + { + return tryReportSuccess(); + } + + // We successfully reached kRunning, but reached kTerminated in error. This affects whether the error + // occurred before or after reaching the ready state + return tryReportError(getErrorAfterState(ProcessState::kRunning)); +} + +IComponent::ComponentError ProcessInfoNode::getErrorAfterState(ProcessState state_reached) const +{ + if (const configuration::ProcessState* state_condition = + std::get_if(&config_.component_properties.ready_condition)) + { + if (*state_condition == configuration::ProcessState::Terminated && state_reached == ProcessState::kRunning) + { + return IComponent::ComponentError::kErrorBeforeReady; + } + } + if (state_reached < ProcessState::kRunning) + { + return IComponent::ComponentError::kErrorBeforeReady; + } + return IComponent::ComponentError::kErrorAfterReady; } void ProcessInfoNode::setupControlClientChannel() @@ -293,16 +343,13 @@ void ProcessInfoNode::setupControlClientChannel() score::cpp::expected_blank ProcessInfoNode::handleProcessStillStarting( const score::cpp::stop_token& stop_token) { - const bool is_native = - config_.component_properties.application_profile.application_type == configuration::ApplicationType::Native; - const bool startup_condition_met = std::visit( - [this, is_native, &stop_token](auto&& arg) -> bool { + [this, &stop_token](auto&& arg) -> bool { using T = std::decay_t; if constexpr (std::is_same_v) { - if (is_native) + if (!isReporting()) { // A native process does not report kRunning, so its exit code is the only readiness indication. return exit_code_ == 0; @@ -316,7 +363,7 @@ score::cpp::expected_blank ProcessInfoNode::handlePr else if constexpr (std::is_same_v) { - if (!is_native) + if (isReporting()) { // currently we do not support multiple ready conditions so we need // to ignore the krunning signal. @@ -359,21 +406,13 @@ score::cpp::expected_blank ProcessInfoNode::handlePr score::cpp::expected_blank ProcessInfoNode::handleProcessAlreadyTerminated() { - if ((0 != exit_code_) || - (configuration::ApplicationType::Native != config_.component_properties.application_profile.application_type)) + // The process did start successfully, but didn't report properly. + if (isReporting()) { - // Error. To get a legal terminated before kRunning the process must be self-terminating, non-reporting - // and to have exited with zero exit code - LM_LOG_WARN() << "Got process termination before kRunning for pid" << pid_ << "(" << identifier_ << ")"; - // This will cause the graph to fail unless we have restart attempts left - return score::cpp::make_unexpected(ComponentError::kErrorBeforeReady); - } - else - { - // case of a self-terminating, non-reporting process exiting nicely before we've had a chance to put an - // entry in the map - return {}; + return score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady); } + // In all other cases, the process did successfully reach the running state (because pid_ was set). + return {}; } score::cpp::expected, IComponent::ComponentError> @@ -384,7 +423,10 @@ ProcessInfoNode::handleProcessStarted(const score::cpp::stop_token& stop_token) case score::mw::lifecycle::internal::SafeProcessMapReturnType::kOk: // Normal case, entry was put in // the map, process still running return handleProcessStillStarting(stop_token); - case score::mw::lifecycle::internal::SafeProcessMapReturnType::kYield: // Process has already exited + case score::mw::lifecycle::internal::SafeProcessMapReturnType::kYield: + // Process has already exited and tryHandleTermination has completed. + // tryHandleTermination is called by insertIfNotTerminated() and therefore executes in sequence in this + // case. return handleProcessAlreadyTerminated(); default: // Error case when pn == -1 // really bad fatal error, should not happen, treat as a failure to set the state & kill the process @@ -396,7 +438,7 @@ ProcessInfoNode::handleProcessStarted(const score::cpp::stop_token& stop_token) void ProcessInfoNode::handleProcessRunning() { - if (configuration::ApplicationType::Native == config_.component_properties.application_profile.application_type) + if (!isReporting()) { LM_LOG_DEBUG() << "Considered kRunning for Non Reporting Process pid" << pid_ << "(" << identifier_ << ")"; } diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp index b059ec246..bd41747ab 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp @@ -40,6 +40,17 @@ namespace score::mw::lifecycle::internal /// In the future, this class shall be split up to properly separate Component and Process lifecycle. class ProcessInfoNode final : public IComponent { + /// @brief Enum representing different outcomes of a termination. + enum class TeminationResult : uint8_t + { + /// @brief Since the last startup, the process has not terminated. + kNone, + /// @brief The last termination was acceptable. + kOk, + /// @brief The last termination was invalid/unexpected. + kError, + }; + public: /// @brief Constructs a ProcessInfoNode. /// @param config Configuration for the OS process. @@ -92,6 +103,13 @@ class ProcessInfoNode final : public IComponent [[nodiscard]] ControlClientChannelP getControlClientChannel() const; private: + /// @brief Given that an error has occurred after the process has reached state @p state_reached, return an error + /// indicating whether this was an error before the ready condition was satisfied, or after. + ComponentError getErrorAfterState(ProcessState state_reached) const; + + /// @brief Returns true if the process is configured to report kRunning + bool isReporting() const; + /// @brief Atomically transitions to new_state if the transition is valid. For reporting /// processes, also notifies the platform health manager of the state change. /// @param new_state The desired process state. @@ -192,6 +210,9 @@ class ProcessInfoNode final : public IComponent /// @brief Unique hash to identify this node. IdentifierHash identifier_; + + /// @brief The result of the last termination since the process started. + TeminationResult termination_result_{}; }; } // namespace score::mw::lifecycle::internal diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp index 0e38a596a..a17986b0a 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp @@ -284,6 +284,130 @@ TEST_F(ProcessInfoNodeStartupTest, ActivateAlreadyActiveNode_ReturnsSuccess) ASSERT_THAT(node->getState(), Eq(score::mw::lifecycle::ProcessState::kRunning)); } +struct YieldTestCasesData +{ + configuration::ApplicationType app_type_; + configuration::ProcessState condition_; + int status_to_exit_with_; + bool is_self_terminating_; + IComponent::RequestResult expected_activation_result_; + std::string description_; +}; + +void PrintTo(const YieldTestCasesData& params, std::ostream* os) +{ + *os << "{ Reporting: " << (params.app_type_ == configuration::ApplicationType::Native ? "No" : "Yes") + << ", Condition: " << (params.condition_ == configuration::ProcessState::Running ? "Running" : "Terminated") + << ", Exit status: " << params.status_to_exit_with_ + << ", Self Terminating: " << (params.is_self_terminating_ ? "Yes" : "No") << " }, " << params.description_; +} + +class ProcessInfoNodeMapYieldTest : public ::WithParamInterface, public ProcessInfoNodeFixture +{ +}; + +TEST_P(ProcessInfoNodeMapYieldTest, InsertReturnsYield) +{ + RecordProperty("Description", GetParam().description_); + + auto node = createProcessInfoNode(GetParam().app_type_, 0, GetParam().is_self_terminating_, GetParam().condition_); + IComponent::RequestResult tryHandleTerminationResult; + auto status = GetParam().status_to_exit_with_; + + EXPECT_CALL(mock_processIf_, startProcess).WillOnce(Return(osal::OsalReturnType::kSuccess)); + // kYield means the process already terminated, so we should not request termination again + EXPECT_CALL(mock_processIf_, requestTermination).Times(0); + EXPECT_CALL(*process_map_, insertIfNotTerminated) + .WillOnce(DoAll( + InvokeWithoutArgs([node = node.get(), &tryHandleTerminationResult, status] { + tryHandleTerminationResult = node->tryHandleTermination(status); + }), + Return(SafeProcessMapReturnType::kYield))); + + auto activation_result_ = node->activate(score::cpp::stop_token{}); + + ASSERT_EQ(activation_result_.has_value(), GetParam().expected_activation_result_.has_value()); + if (GetParam().expected_activation_result_.has_value()) + { + EXPECT_EQ(activation_result_.value(), GetParam().expected_activation_result_.value()); + } + else + { + EXPECT_EQ(activation_result_.error(), GetParam().expected_activation_result_.error()); + } + + ASSERT_TRUE(tryHandleTerminationResult.has_value()); + EXPECT_EQ(tryHandleTerminationResult.value(), IComponent::RequestState::kWaiting) + << "An error occurring during startup should never be reported by tryHandleTermination"; + EXPECT_EQ(node->getState(), ProcessState::kTerminated); // Not kFailed, the posix process did start successfully +} + +INSTANTIATE_TEST_SUITE_P( + ProcessInfoNodeTest, + ProcessInfoNodeMapYieldTest, + Values( + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Running, + 0, + true, + {IComponent::RequestState::kSuccess}, + "A native, self-terminating process exiting quickly with status 0 should report a successful activation"}, + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Running, + 111, + true, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorAfterReady), + "A native, self-terminating process exiting quickly with a non-zero status should report a failure to " + "activate"}, + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Running, + 0, + false, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorAfterReady), + "A native, non-self-terminating process exiting quickly should report a failure to activate"}, + YieldTestCasesData{ + configuration::ApplicationType::Reporting, + configuration::ProcessState::Running, + 0, + true, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), + "A reporting process exiting quickly (i.e. without waiting for a response from launch manager) should " + "report a failure to activate"}, + + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Terminated, + 0, + true, + {IComponent::RequestState::kSuccess}, + "A native, self-terminating process exiting quickly with status 0 should report a successful activation"}, + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Terminated, + 111, + true, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), + "A native, self-terminating process exiting quickly with a non-zero status should report a failure to " + "activate"}, + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Terminated, + 0, + false, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), + "A native, non-self-terminating process exiting quickly should report a failure to activate"}, + YieldTestCasesData{ + configuration::ApplicationType::Reporting, + configuration::ProcessState::Terminated, + 0, + true, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), + "A reporting process exiting quickly (i.e. without waiting for a response from launch manager) should " + "report a failure to activate"})); + // Bundles process crashes and timeouts that occur during activate(), before the ready condition is reached. class ProcessInfoNodeStartupCrashTest : public ProcessInfoNodeFixture { diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_monitor.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_monitor.cpp index 1b7401b18..3f8e6a79d 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_monitor.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_monitor.cpp @@ -116,7 +116,7 @@ void ProcessMonitor::terminated(IComponent& component, int32_t status) bool push_res = true; if (!res.has_value()) { - push_res = event_queue_.push(UnexpectedTermination{component.getIdentifier()}); + push_res = event_queue_.push(UnexpectedTermination{component.getIdentifier(), res.error()}); } else if (res.value() != IComponent::RequestState::kWaiting) { diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.cpp index d023fe8d0..a3d063bec 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.cpp @@ -25,33 +25,33 @@ SafeProcessMap::SafeProcessMap(uint32_t capacity, IComponentController& terminat { if (capacity) { - free_root_ = 0U; - pid_root_.store(LINK_NO_VALUE); + free_list_head_ = 0U; + tree_root_.store(NULL_INDEX); for (std::size_t i = 0U; i < capacity; ++i) { items_[i].pid_ = 0; - items_[i].data_.pin_ = nullptr; - items_[i].pid_left_ = LINK_NO_VALUE; + items_[i].data_.component_ = nullptr; + items_[i].pid_left_ = NULL_INDEX; items_[i].pid_right_ = static_cast(i + 1U); items_[i].data_.status_ = -1; } - items_[capacity - 1UL].pid_right_ = LINK_NO_VALUE; + items_[capacity - 1UL].pid_right_ = NULL_INDEX; } } -void SafeProcessMap::findNode(uint32_t& mask, uint32_t& last, osal::ProcessID key) +void SafeProcessMap::findNode(uint32_t& mask, uint32_t& parent, osal::ProcessID key) { - while (rover_ != LINK_NO_VALUE && key != items_[rover_].pid_) + while (current_ != NULL_INDEX && key != items_[current_].pid_) { - last = rover_; + parent = current_; if (static_cast(key) & mask) { - rover_ = items_[last].pid_left_; + current_ = items_[parent].pid_left_; } else { - rover_ = items_[last].pid_right_; + current_ = items_[parent].pid_right_; } mask = mask << 1U; @@ -67,13 +67,13 @@ void SafeProcessMap::findNode(uint32_t& mask, uint32_t& last, osal::ProcessID ke } // RULECHECKER_comment(1, 1, check_max_parameters, "refactored with WI #9343", true); -int32_t SafeProcessMap::insertNode(uint32_t& mask, uint32_t& last, osal::ProcessID& key, ProcessInfoData& data) +int32_t SafeProcessMap::insertNode(uint32_t& mask, uint32_t& parent, osal::ProcessID& key, ProcessInfoData& data) { int32_t ret_value = -1; - rover_ = free_root_; + current_ = free_list_head_; - if (rover_ == LINK_NO_VALUE) + if (current_ == NULL_INDEX) { // too bad, we are out of memory ret_value = -1; @@ -81,22 +81,22 @@ int32_t SafeProcessMap::insertNode(uint32_t& mask, uint32_t& last, osal::Process else { mask = mask >> 1U; - free_root_ = items_[rover_].pid_right_; - items_[rover_].pid_ = key; - items_[rover_].data_ = data; - items_[rover_].pid_left_ = LINK_NO_VALUE; - items_[rover_].pid_right_ = LINK_NO_VALUE; + free_list_head_ = items_[current_].pid_right_; + items_[current_].pid_ = key; + items_[current_].data_ = data; + items_[current_].pid_left_ = NULL_INDEX; + items_[current_].pid_right_ = NULL_INDEX; if (static_cast(key) & mask) { - items_[last].pid_left_ = rover_; + items_[parent].pid_left_ = current_; } else { - items_[last].pid_right_ = rover_; + items_[parent].pid_right_ = current_; } - if (data.pin_ == nullptr) + if (data.component_ == nullptr) { ret_value = 1; } @@ -110,29 +110,29 @@ int32_t SafeProcessMap::insertNode(uint32_t& mask, uint32_t& last, osal::Process } // RULECHECKER_comment(1, 1, check_max_parameters, "refactored with WI #9343", true); -int32_t SafeProcessMap::removeNode(ProcessInfoData& target, ProcessInfoData& data, uint32_t& last, uint32_t& local_root) +int32_t SafeProcessMap::removeNode(ProcessInfoData& target, ProcessInfoData& data, uint32_t& parent, uint32_t& root) { // found key. There are 4 situations: - // data.pin_ == nullptr, stored pin_ != nullptr: normal findTerminated - // data.pin_ != nullptr, stored pin_ == nullptr: normal insertIfNotTerminated - // both data.pin_ and stored pin_ point to an ITerminationCallback: anomalous - // both data.pin_ and stored pin_ are null: anomalous - // In other words, exactly one of data.pin_ and stored pin_ must be nullptr + // data.component_ == nullptr, stored component_ != nullptr: normal findTerminated + // data.component_ != nullptr, stored component_ == nullptr: normal insertIfNotTerminated + // both data.component_ and stored component_ point to an ITerminationCallback: anomalous + // both data.component_ and stored component_ are null: anomalous + // In other words, exactly one of data.component_ and stored component_ must be nullptr // or there is an anomaly and we return -2 int32_t ret_value = -2; - if ((nullptr == data.pin_) ^ (nullptr == items_[rover_].data_.pin_)) + if ((nullptr == data.component_) ^ (nullptr == items_[current_].data_.component_)) { // found key, we will remove it! - target = items_[rover_].data_; - if (target.pin_) + target = items_[current_].data_; + if (target.component_) { target.status_ = data.status_; } else { - target.pin_ = data.pin_; + target.component_ = data.component_; } - if (data.pin_) + if (data.component_) { ret_value = 1; } @@ -141,26 +141,26 @@ int32_t SafeProcessMap::removeNode(ProcessInfoData& target, ProcessInfoData& dat ret_value = 0; } // Need to find a suitable leaf to use as the replacement - uint32_t leaf = rover_; - uint32_t previous = rover_; - findLeaf(leaf, previous); - deleteNode(last, leaf, local_root, previous); + uint32_t leaf = current_; + uint32_t leaf_parent = current_; + findLeaf(leaf, leaf_parent); + deleteNode(parent, leaf, root, leaf_parent); } return ret_value; } -void SafeProcessMap::findLeaf(uint32_t& leaf, uint32_t& previous) +void SafeProcessMap::findLeaf(uint32_t& leaf, uint32_t& leaf_parent) { while (true) { - if (items_[leaf].pid_left_ != LINK_NO_VALUE) + if (items_[leaf].pid_left_ != NULL_INDEX) { - previous = leaf; + leaf_parent = leaf; leaf = items_[leaf].pid_left_; } - else if (items_[leaf].pid_right_ != LINK_NO_VALUE) + else if (items_[leaf].pid_right_ != NULL_INDEX) { - previous = leaf; + leaf_parent = leaf; leaf = items_[leaf].pid_right_; } else @@ -171,50 +171,50 @@ void SafeProcessMap::findLeaf(uint32_t& leaf, uint32_t& previous) } // RULECHECKER_comment(1, 1, check_max_parameters, "refactored with WI #9343", true); -void SafeProcessMap::deleteNode(uint32_t& last, uint32_t& leaf, uint32_t& local_root, uint32_t& previous) +void SafeProcessMap::deleteNode(uint32_t& parent, uint32_t& leaf, uint32_t& root, uint32_t& leaf_parent) { - if (leaf == local_root) + if (leaf == root) { // tree is now empty! - local_root = LINK_NO_VALUE; + root = NULL_INDEX; } else { - if (leaf == rover_) + if (leaf == current_) { - // simply remove the link to rover - if (items_[last].pid_left_ == rover_) + // simply remove the link to current_ + if (items_[parent].pid_left_ == current_) { - items_[last].pid_left_ = LINK_NO_VALUE; + items_[parent].pid_left_ = NULL_INDEX; } else { - items_[last].pid_right_ = LINK_NO_VALUE; + items_[parent].pid_right_ = NULL_INDEX; } } else { // Put the leaf in place of the item we are replacing - items_[rover_].pid_ = items_[leaf].pid_; - items_[rover_].data_ = items_[leaf].data_; + items_[current_].pid_ = items_[leaf].pid_; + items_[current_].data_ = items_[leaf].data_; // Remove the links on the item that previously pointed to the leaf - if (items_[previous].pid_left_ == leaf) + if (items_[leaf_parent].pid_left_ == leaf) { - items_[previous].pid_left_ = LINK_NO_VALUE; + items_[leaf_parent].pid_left_ = NULL_INDEX; } else { - items_[previous].pid_right_ = LINK_NO_VALUE; + items_[leaf_parent].pid_right_ = NULL_INDEX; } } } // now return the leaf we found to the free list items_[leaf].pid_ = 0; items_[leaf].data_ = {-1, nullptr}; - items_[leaf].pid_left_ = LINK_NO_VALUE; - items_[leaf].pid_right_ = free_root_; - free_root_ = leaf; + items_[leaf].pid_left_ = NULL_INDEX; + items_[leaf].pid_right_ = free_list_head_; + free_list_head_ = leaf; } int32_t SafeProcessMap::search(osal::ProcessID key, ProcessInfoData data) @@ -228,27 +228,27 @@ int32_t SafeProcessMap::search(osal::ProcessID key, ProcessInfoData data) ProcessInfoData target; target = {data.status_, nullptr}; // Gain a lock on the root of the tree - uint32_t local_root = pid_root_.exchange(LINK_LOCKED); + uint32_t root = tree_root_.exchange(LOCKED_INDEX); - while (local_root == LINK_LOCKED) + while (root == LOCKED_INDEX) { std::this_thread::yield(); - local_root = pid_root_.exchange(LINK_LOCKED); + root = tree_root_.exchange(LOCKED_INDEX); } - rover_ = local_root; + current_ = root; - if (local_root == LINK_NO_VALUE) + if (root == NULL_INDEX) { // no tree, special case. - rover_ = free_root_; - free_root_ = items_[rover_].pid_right_; - items_[rover_].pid_ = key; - items_[rover_].data_ = data; - items_[rover_].pid_left_ = LINK_NO_VALUE; - items_[rover_].pid_right_ = LINK_NO_VALUE; - local_root = rover_; + current_ = free_list_head_; + free_list_head_ = items_[current_].pid_right_; + items_[current_].pid_ = key; + items_[current_].data_ = data; + items_[current_].pid_left_ = NULL_INDEX; + items_[current_].pid_right_ = NULL_INDEX; + root = current_; - if (data.pin_ == nullptr) + if (data.component_ == nullptr) { ret_value = 1; } @@ -260,33 +260,33 @@ int32_t SafeProcessMap::search(osal::ProcessID key, ProcessInfoData data) else { // Look for the key - uint32_t last = LINK_NO_VALUE; + uint32_t parent = NULL_INDEX; uint32_t mask = 1U; - findNode(mask, last, key); + findNode(mask, parent, key); - if (rover_ == LINK_NO_VALUE) + if (current_ == NULL_INDEX) { // key not found, we will add it - ret_value = insertNode(mask, last, key, data); + ret_value = insertNode(mask, parent, key, data); } else { // found key, we will remove it! - ret_value = removeNode(target, data, last, local_root); + ret_value = removeNode(target, data, parent, root); } } // release the lock on the tree - pid_root_.store(local_root); + tree_root_.store(root); if (-2 == ret_value) { // allow another thread to run to resolve the anomaly std::this_thread::yield(); } - else if (target.pin_) + else if (target.component_) { - termination_handler_.terminated(*target.pin_, target.status_); + termination_handler_.terminated(*target.component_, target.status_); } } } diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.hpp index ac83cdd77..8c051945c 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.hpp @@ -25,8 +25,8 @@ namespace score::mw::lifecycle::internal /// @brief Struct representing data in a map item struct ProcessInfoData { - int32_t status_ = -1; ///< Exit status for process - IComponent* pin_ = nullptr; ///< Pointer to the termination callback associated with this item. + int32_t status_ = -1; ///< Exit status for process + IComponent* component_ = nullptr; ///< Pointer to the termination callback associated with this item. }; /// @brief Struct representing an item in the map. struct ProcessTreeNode @@ -138,7 +138,7 @@ class SafeProcessMap final : public SafeProcessMapInserter /// @param last Reference to an integer where the index of the last visited node will be stored. /// This parameter is updated during the traversal to keep track of the last node visited. /// @param key The process ID to find in the tree. - void findNode(uint32_t& mask, uint32_t& last, osal::ProcessID key); + void findNode(uint32_t& mask, uint32_t& parent, osal::ProcessID key); /// @brief Inserts a node into the SafeProcessMap with the given process ID and associated information. /// This function inserts a node into the SafeProcessMap using a rover mechanism for safe traversal and insertion. @@ -154,7 +154,7 @@ class SafeProcessMap final : public SafeProcessMapInserter /// - 0 if the node was successfully inserted. /// - 1 if the insertion was successful but the object pointer was null. /// - -1 if the insertion failed due to memory constraints (out of memory). - int32_t insertNode(uint32_t& mask, uint32_t& last, osal::ProcessID& key, ProcessInfoData& data); + int32_t insertNode(uint32_t& mask, uint32_t& parent, osal::ProcessID& key, ProcessInfoData& data); /// @brief Removes a node from the process map tree. /// This function removes the node currently pointed to by the rover in the SafeProcessMap. @@ -167,7 +167,7 @@ class SafeProcessMap final : public SafeProcessMapInserter /// @param local_root Index of the first object /// @return int32_t Returns 0 if the node was successfully removed and `object` was nullptr, /// 1 if `object` was not nullptr, and -2 if the removal failed due to PID re-use - int32_t removeNode(ProcessInfoData& target, ProcessInfoData& data, uint32_t& last, uint32_t& local_root); + int32_t removeNode(ProcessInfoData& target, ProcessInfoData& data, uint32_t& parent, uint32_t& root); /// @brief Finds the leaf node in the SafeProcessMap starting from the given node. /// This function traverses the SafeProcessMap starting from the specified node `leaf` @@ -176,7 +176,7 @@ class SafeProcessMap final : public SafeProcessMapInserter /// node. Upon successful execution, this parameter will store the index of the found leaf node. /// @param previous Reference to an integer that will store the index of the parent node of the found leaf node. /// If the `leaf` node itself is the root or a leaf, `previous` will be set to the same as `leaf`. - void findLeaf(uint32_t& leaf, uint32_t& previous); + void findLeaf(uint32_t& leaf, uint32_t& leaf_parent); /// @brief Deletes a node from the SafeProcessMap, handling reorganization and freeing of resources. /// This function deletes a node from the SafeProcessMap structure based on the given parameters, @@ -186,30 +186,30 @@ class SafeProcessMap final : public SafeProcessMapInserter /// @param leaf Reference to an integer representing the index of the node to be deleted from the map. /// Upon successful deletion, this parameter will be returned to the free list for reuse. /// @param local_root Reference to an integer storing the index of the root node of the current tree structure. - /// If the deleted node is the root, this parameter is updated to LINK_NO_VALUE, indicating an empty tree. + /// If the deleted node is the root, this parameter is updated to NULL_INDEX, indicating an empty tree. /// @param previous Reference to an integer storing the index of the parent node of the deleted node. /// This parameter is used to update the link of the parent node after deletion. - void deleteNode(uint32_t& last, uint32_t& leaf, uint32_t& local_root, uint32_t& previous); + void deleteNode(uint32_t& parent, uint32_t& leaf, uint32_t& root, uint32_t& leaf_parent); ///@brief Unique pointer managing an array of ProcessTreeNode objects. std::unique_ptr items_; /// @brief Value indicating that no node is assigned. - static constexpr uint32_t LINK_NO_VALUE = 0xFFFFFFFF; + static constexpr uint32_t NULL_INDEX = 0xFFFFFFFF; /// @brief Value indicating that a node is locked. - static constexpr uint32_t LINK_LOCKED = 0xFFFFFFFE; + static constexpr uint32_t LOCKED_INDEX = 0xFFFFFFFE; /// @brief Root of the binary tree used to find an entry by process ID (pid). - std::atomic_uint32_t pid_root_{LINK_NO_VALUE}; + std::atomic_uint32_t tree_root_{NULL_INDEX}; /// @brief Root of the list of free entries. - uint32_t free_root_{LINK_NO_VALUE}; + uint32_t free_list_head_{NULL_INDEX}; /// @brief Current rover index in the SafeProcessMap. /// This variable represents the current index used for traversal within the SafeProcessMap. - /// It initially starts with LINK_NO_VALUE, indicating no valid position. - uint32_t rover_{LINK_NO_VALUE}; + /// It initially starts with NULL_INDEX, indicating no valid position. + uint32_t current_{NULL_INDEX}; IComponentController& termination_handler_; };