From 435d07eaf95ed3c412dd89e0b5aa9f81c57b4fa1 Mon Sep 17 00:00:00 2001 From: Dennis Klein Date: Tue, 28 Feb 2023 12:12:54 +0100 Subject: [PATCH] feat: Improve `ChangeState` API * Add `[[nodiscard]]` to `bool Device::ChangeState()` * Introduce throwing variant `void Device::ChangeStateOrThrow()` resolves #441 --- fairmq/Device.cxx | 40 ++++++++++++++++++------------------ fairmq/Device.h | 33 +++++++++++++++++++++++++++-- fairmq/DeviceRunner.cxx | 4 ++-- test/device/_transitions.cxx | 22 ++++++++++++++++++++ 4 files changed, 75 insertions(+), 24 deletions(-) diff --git a/fairmq/Device.cxx b/fairmq/Device.cxx index 93a5a966..fa267f53 100644 --- a/fairmq/Device.cxx +++ b/fairmq/Device.cxx @@ -164,30 +164,30 @@ void Device::TransitionTo(State s) while (s != currentState) { switch (currentState) { case State::Idle: - if (s == State::Exiting) { ChangeState(Transition::End); } - else { ChangeState(Transition::InitDevice); } + if (s == State::Exiting) { ChangeStateOrThrow(Transition::End); } + else { ChangeStateOrThrow(Transition::InitDevice); } break; case State::InitializingDevice: - ChangeState(Transition::CompleteInit); + ChangeStateOrThrow(Transition::CompleteInit); break; case State::Initialized: - if (s == State::Exiting || s == State::Idle) { ChangeState(Transition::ResetDevice); } - else { ChangeState(Transition::Bind); } + if (s == State::Exiting || s == State::Idle) { ChangeStateOrThrow(Transition::ResetDevice); } + else { ChangeStateOrThrow(Transition::Bind); } break; case State::Bound: - if (s == State::DeviceReady || s == State::Ready || s == State::Running) { ChangeState(Transition::Connect); } - else { ChangeState(Transition::ResetDevice); } + if (s == State::DeviceReady || s == State::Ready || s == State::Running) { ChangeStateOrThrow(Transition::Connect); } + else { ChangeStateOrThrow(Transition::ResetDevice); } break; case State::DeviceReady: - if (s == State::Running || s == State::Ready) { ChangeState(Transition::InitTask); } - else { ChangeState(Transition::ResetDevice); } + if (s == State::Running || s == State::Ready) { ChangeStateOrThrow(Transition::InitTask); } + else { ChangeStateOrThrow(Transition::ResetDevice); } break; case State::Ready: - if (s == State::Running) { ChangeState(Transition::Run); } - else { ChangeState(Transition::ResetTask); } + if (s == State::Running) { ChangeStateOrThrow(Transition::Run); } + else { ChangeStateOrThrow(Transition::ResetTask); } break; case State::Running: - ChangeState(Transition::Stop); + ChangeStateOrThrow(Transition::Stop); break; case State::Binding: case State::Connecting: @@ -281,7 +281,7 @@ void Device::InitWrapper() } } - // ChangeState(Transition::Auto); + // ChangeStateOrThrow(Transition::Auto); } void Device::BindWrapper() @@ -298,7 +298,7 @@ void Device::BindWrapper() Bind(); if (!NewStatePending()) { - ChangeState(Transition::Auto); + ChangeStateOrThrow(Transition::Auto); } } @@ -341,7 +341,7 @@ void Device::ConnectWrapper() Connect(); if (!NewStatePending()) { - ChangeState(Transition::Auto); + ChangeStateOrThrow(Transition::Auto); } } @@ -443,7 +443,7 @@ void Device::InitTaskWrapper() InitTask(); if (!NewStatePending()) { - ChangeState(Transition::Auto); + ChangeStateOrThrow(Transition::Auto); } } @@ -466,7 +466,7 @@ void Device::RunWrapper() // change to Error state in case of an exception, to release LogSocketRates tools::CallOnDestruction cod([&](){ - ChangeState(Transition::ErrorFound); + ChangeStateOrThrow(Transition::ErrorFound); }); PreRun(); @@ -493,7 +493,7 @@ void Device::RunWrapper() // if Run() exited and the state is still RUNNING, transition to READY. if (!NewStatePending()) { - ChangeState(Transition::Stop); + ChangeStateOrThrow(Transition::Stop); } PostRun(); @@ -794,7 +794,7 @@ void Device::ResetTaskWrapper() ResetTask(); if (!NewStatePending()) { - ChangeState(Transition::Auto); + ChangeStateOrThrow(Transition::Auto); } } @@ -813,7 +813,7 @@ void Device::ResetWrapper() GetChannels().clear(); fTransportFactory.reset(); if (!NewStatePending()) { - ChangeState(Transition::Auto); + ChangeStateOrThrow(Transition::Auto); } } diff --git a/fairmq/Device.h b/fairmq/Device.h index 1fab7c65..8f95b499 100644 --- a/fairmq/Device.h +++ b/fairmq/Device.h @@ -11,6 +11,7 @@ // FairMQ #include +#include #include #include #include @@ -498,21 +499,49 @@ class Device public: /// @brief Request a device state transition /// @param transition state transition + /// @return whether the transition was successfully scheduled /// /// The state transition may not happen immediately, but when the current state evaluates the /// pending transition event and terminates. In other words, the device states are scheduled /// cooperatively. - bool ChangeState(const Transition transition) { return fStateMachine.ChangeState(transition); } + [[nodiscard]] bool ChangeState(const Transition transition) + { + return fStateMachine.ChangeState(transition); + } /// @brief Request a device state transition /// @param transition state transition + /// @return whether the transition was successfully scheduled /// /// The state transition may not happen immediately, but when the current state evaluates the /// pending transition event and terminates. In other words, the device states are scheduled /// cooperatively. - bool ChangeState(const std::string& transition) + [[nodiscard]] bool ChangeState(const std::string& transition) { return fStateMachine.ChangeState(GetTransition(transition)); } + /// @brief Request a device state transition + /// @param transition state transition + /// @throws when the transition could not have been scheduled + /// + /// Throwing version of Device::ChangeState(). + void ChangeStateOrThrow(Transition transition) + { + if(!ChangeState(transition)) { + auto const err = MakeErrorCode(ErrorCode::DeviceChangeStateFailed); + throw std::system_error(err.value(), + err.category(), + tools::ToString("Invalid transition: ", transition)); + } + } + /// @brief Request a device state transition + /// @param transition state transition + /// @throws when the transition could not have been scheduled + /// + /// Throwing version of Device::ChangeState(). + void ChangeStateOrThrow(std::string const& transition) + { + ChangeStateOrThrow(GetTransition(transition)); + } /// @brief waits for the next state (any) to occur State WaitForNextState() { return fStateQueue.WaitForNext(); } diff --git a/fairmq/DeviceRunner.cxx b/fairmq/DeviceRunner.cxx index c9173cc5..e31eae5d 100644 --- a/fairmq/DeviceRunner.cxx +++ b/fairmq/DeviceRunner.cxx @@ -152,7 +152,7 @@ auto DeviceRunner::Run() -> int fDevice->RegisterChannelEndpoints(); if (fConfig.Count("print-channels")) { fDevice->PrintRegisteredChannels(); - fDevice->ChangeState(fair::mq::Transition::End); + fDevice->ChangeStateOrThrow(fair::mq::Transition::End); return 0; } @@ -160,7 +160,7 @@ auto DeviceRunner::Run() -> int if (fConfig.Count("version")) { LOGV(info, verylow) << "FairMQ version: " << FAIRMQ_GIT_VERSION; LOGV(info, verylow) << "User device version: " << fDevice->GetVersion(); - fDevice->ChangeState(fair::mq::Transition::End); + fDevice->ChangeStateOrThrow(fair::mq::Transition::End); return 0; } diff --git a/test/device/_transitions.cxx b/test/device/_transitions.cxx index 00ad6fb7..e1eea343 100644 --- a/test/device/_transitions.cxx +++ b/test/device/_transitions.cxx @@ -97,4 +97,26 @@ TEST(Transitions, ConcurrentTransitionTos) } } +TEST(Transitions, InvalidChangeState) +{ + Device device; + thread t([&] { device.RunStateMachine(); }); + + ASSERT_FALSE(device.ChangeState(Transition::Connect)); + + ASSERT_TRUE(device.ChangeState(Transition::End)); + if (t.joinable()) { t.join(); } +} + +TEST(Transitions, InvalidChangeStateOrThrow) +{ + Device device; + thread t([&] { device.RunStateMachine(); }); + + ASSERT_THROW(device.ChangeStateOrThrow(Transition::Connect), std::system_error); + + ASSERT_NO_THROW(device.ChangeStateOrThrow(Transition::End)); + if (t.joinable()) { t.join(); } +} + } // namespace