From 2624083c1fdc2bbcea6880a217b881e07acf031a Mon Sep 17 00:00:00 2001 From: Nick Le Large Date: Mon, 7 Oct 2024 15:27:40 +0200 Subject: [PATCH 1/3] Wrap getAndVerifyCommand in try catch block Includes a basic unit test --- .../internal/arbitrator_impl.hpp | 10 +++- test/dummy_types.hpp | 10 ++++ test/handle_exceptions.cpp | 51 +++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 test/handle_exceptions.cpp diff --git a/include/arbitration_graphs/internal/arbitrator_impl.hpp b/include/arbitration_graphs/internal/arbitrator_impl.hpp index 5ec6f5ac..a3147359 100644 --- a/include/arbitration_graphs/internal/arbitrator_impl.hpp +++ b/include/arbitration_graphs/internal/arbitrator_impl.hpp @@ -110,7 +110,15 @@ SubCommandT Arbitrator::g // otherwise we have bestOption == activeBehavior_ which already gained control // an arbitrator as option might not return a command, if its applicable options fail verification: - const std::optional command = getAndVerifyCommand(bestOption, time); + std::optional command; + try { + command = getAndVerifyCommand(bestOption, time); + } catch (const std::exception& e) { + VLOG(1) << bestOption->behavior_->name_ << " threw an exception during getAndVerifyCommand(): " << e.what(); + bestOption->verificationResult_.reset(); + bestOption->behavior_->loseControl(time); + continue; + } if (command) { if (activeBehavior_ && bestOption != activeBehavior_) { diff --git a/test/dummy_types.hpp b/test/dummy_types.hpp index 37126a5e..8c122226 100644 --- a/test/dummy_types.hpp +++ b/test/dummy_types.hpp @@ -63,6 +63,16 @@ class DummyBehavior : public Behavior { int loseControlCounter_{0}; }; +class BrokenDummyBehavior : public DummyBehavior { +public: + BrokenDummyBehavior(const bool invocation, const bool commitment, const std::string& name = "BrokenDummyBehavior") + : DummyBehavior(invocation, commitment, name) {}; + + DummyCommand getCommand(const Time& time) override { + throw std::runtime_error("BrokenDummyBehavior::getCommand() is broken"); + } +}; + struct DummyResult { bool isOk() const { return isOk_; diff --git a/test/handle_exceptions.cpp b/test/handle_exceptions.cpp new file mode 100644 index 00000000..2462f838 --- /dev/null +++ b/test/handle_exceptions.cpp @@ -0,0 +1,51 @@ +#include +#include +#include "gtest/gtest.h" + +#include "behavior.hpp" +#include "priority_arbitrator.hpp" + +#include "dummy_types.hpp" + +using namespace arbitration_graphs; +using namespace arbitration_graphs_tests; + +class ExceptionHandlingTest : public ::testing::Test { +protected: + BrokenDummyBehavior::Ptr testBehaviorHighPriority = + std::make_shared(true, true, "HighPriority"); + DummyBehavior::Ptr testBehaviorLowPriority = std::make_shared(true, true, "LowPriority"); + + Time time{Clock::now()}; +}; + +TEST_F(ExceptionHandlingTest, HandleException) { + using OptionFlags = PriorityArbitrator::Option::Flags; + + PriorityArbitrator testPriorityArbitrator; + + testPriorityArbitrator.addOption(testBehaviorHighPriority, OptionFlags::NO_FLAGS); + testPriorityArbitrator.addOption(testBehaviorLowPriority, OptionFlags::NO_FLAGS); + + ASSERT_TRUE(testPriorityArbitrator.checkInvocationCondition(time)); + + testPriorityArbitrator.gainControl(time); + + // Since the high priority behavior is broken, we should get the low priority behavior as fallback + EXPECT_EQ("LowPriority", testPriorityArbitrator.getCommand(time)); + EXPECT_FALSE(testPriorityArbitrator.options().at(0)->verificationResult_.cached(time)); + ASSERT_TRUE(testPriorityArbitrator.options().at(1)->verificationResult_.cached(time)); + + EXPECT_TRUE(testPriorityArbitrator.options().at(1)->verificationResult_.cached(time)->isOk()); + + testPriorityArbitrator.loseControl(time); + + testBehaviorLowPriority->invocationCondition_ = false; + ASSERT_TRUE(testPriorityArbitrator.checkInvocationCondition(time)); + + testPriorityArbitrator.gainControl(time); + + // With no fallback, there is no option to call even if the invocation condition is true + EXPECT_THROW(testPriorityArbitrator.getCommand(time), NoApplicableOptionPassedVerificationError); +} + From b9e2d6f67cff299e86f6c754bb8ea52e77b8c819 Mon Sep 17 00:00:00 2001 From: Piotr Spieker Date: Tue, 19 Nov 2024 23:38:10 +0100 Subject: [PATCH 2/3] Force verification results to keep a isOk_ member or provide a bool constructor --- include/arbitration_graphs/verification.hpp | 6 ++++-- test/dummy_types.hpp | 11 +++-------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/include/arbitration_graphs/verification.hpp b/include/arbitration_graphs/verification.hpp index 9a1316f5..b0a2a8c7 100644 --- a/include/arbitration_graphs/verification.hpp +++ b/include/arbitration_graphs/verification.hpp @@ -14,9 +14,11 @@ namespace arbitration_graphs::verification { * robots. Otherwise these are a good starting point to implement your own meaningful verifier. */ struct PlaceboResult { - static bool isOk() { - return true; + bool isOk() const { + return isOk_; }; + + bool isOk_{true}; }; template struct PlaceboVerifier { diff --git a/test/dummy_types.hpp b/test/dummy_types.hpp index 8c122226..9fb5dbfd 100644 --- a/test/dummy_types.hpp +++ b/test/dummy_types.hpp @@ -1,6 +1,7 @@ #pragma once #include "behavior.hpp" +#include "verification.hpp" namespace arbitration_graphs_tests { @@ -66,20 +67,14 @@ class DummyBehavior : public Behavior { class BrokenDummyBehavior : public DummyBehavior { public: BrokenDummyBehavior(const bool invocation, const bool commitment, const std::string& name = "BrokenDummyBehavior") - : DummyBehavior(invocation, commitment, name) {}; + : DummyBehavior(invocation, commitment, name){}; DummyCommand getCommand(const Time& time) override { throw std::runtime_error("BrokenDummyBehavior::getCommand() is broken"); } }; -struct DummyResult { - bool isOk() const { - return isOk_; - }; - - bool isOk_; -}; +struct DummyResult : public verification::PlaceboResult {}; } // namespace arbitration_graphs_tests From d36718937f527924569b18a448e8f6559b45650c Mon Sep 17 00:00:00 2001 From: Piotr Spieker Date: Tue, 19 Nov 2024 23:39:21 +0100 Subject: [PATCH 3/3] Set the verification result to not Ok, when the arbitrator catched an exception --- include/arbitration_graphs/internal/arbitrator_impl.hpp | 2 +- test/handle_exceptions.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/arbitration_graphs/internal/arbitrator_impl.hpp b/include/arbitration_graphs/internal/arbitrator_impl.hpp index a3147359..deb73023 100644 --- a/include/arbitration_graphs/internal/arbitrator_impl.hpp +++ b/include/arbitration_graphs/internal/arbitrator_impl.hpp @@ -115,7 +115,7 @@ SubCommandT Arbitrator::g command = getAndVerifyCommand(bestOption, time); } catch (const std::exception& e) { VLOG(1) << bestOption->behavior_->name_ << " threw an exception during getAndVerifyCommand(): " << e.what(); - bestOption->verificationResult_.reset(); + bestOption->verificationResult_.cache(time, VerificationResultT{false}); bestOption->behavior_->loseControl(time); continue; } diff --git a/test/handle_exceptions.cpp b/test/handle_exceptions.cpp index 2462f838..10afa598 100644 --- a/test/handle_exceptions.cpp +++ b/test/handle_exceptions.cpp @@ -33,9 +33,10 @@ TEST_F(ExceptionHandlingTest, HandleException) { // Since the high priority behavior is broken, we should get the low priority behavior as fallback EXPECT_EQ("LowPriority", testPriorityArbitrator.getCommand(time)); - EXPECT_FALSE(testPriorityArbitrator.options().at(0)->verificationResult_.cached(time)); + ASSERT_TRUE(testPriorityArbitrator.options().at(0)->verificationResult_.cached(time)); ASSERT_TRUE(testPriorityArbitrator.options().at(1)->verificationResult_.cached(time)); + EXPECT_FALSE(testPriorityArbitrator.options().at(0)->verificationResult_.cached(time)->isOk()); EXPECT_TRUE(testPriorityArbitrator.options().at(1)->verificationResult_.cached(time)->isOk()); testPriorityArbitrator.loseControl(time);