From 590c07ad8473f497130806f51717c81bfacc294c Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Thu, 9 Jan 2025 15:26:25 +0000 Subject: [PATCH] fix: AsyncFramework RAII (#1815) Fixes #1812 --- src/util/MoveTracker.hpp | 78 +++++++++++++++++++ src/util/async/Operation.hpp | 42 ++++++++-- tests/unit/CMakeLists.txt | 6 +- tests/unit/util/MoveTrackerTests.cpp | 68 ++++++++++++++++ .../util/async/AsyncExecutionContextTests.cpp | 70 ++++++++++++++++- 5 files changed, 255 insertions(+), 9 deletions(-) create mode 100644 src/util/MoveTracker.hpp create mode 100644 tests/unit/util/MoveTrackerTests.cpp diff --git a/src/util/MoveTracker.hpp b/src/util/MoveTracker.hpp new file mode 100644 index 000000000..22d0bd8ae --- /dev/null +++ b/src/util/MoveTracker.hpp @@ -0,0 +1,78 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#pragma once + +#include + +namespace util { + +/** + * @brief A base-class that can be used to check whether the current instance was moved from + */ +class MoveTracker { + bool wasMoved_ = false; + +protected: + /** + * @brief The function to be used by clients in order to check whether the instance was moved from + * @return true if moved from; false otherwise + */ + [[nodiscard]] bool + wasMoved() const noexcept + { + return wasMoved_; + } + + MoveTracker() = default; // should only be used via inheritance + +public: + virtual ~MoveTracker() = default; + + /** + * @brief Move constructor sets the moved-from state on `other` and resets the state on `this` + * @param other The moved-from object + */ + MoveTracker(MoveTracker&& other) + { + *this = std::move(other); + } + + /** + * @brief Move operator sets the moved-from state on `other` and resets the state on `this` + * @param other The moved-from object + * @return Reference to self + */ + MoveTracker& + operator=(MoveTracker&& other) + { + if (this != &other) { + other.wasMoved_ = true; + wasMoved_ = false; + } + + return *this; + } + + MoveTracker(MoveTracker const&) = default; + MoveTracker& + operator=(MoveTracker const&) = default; +}; + +} // namespace util diff --git a/src/util/async/Operation.hpp b/src/util/async/Operation.hpp index 742e346cd..1ad0c41fe 100644 --- a/src/util/async/Operation.hpp +++ b/src/util/async/Operation.hpp @@ -19,9 +19,9 @@ #pragma once +#include "util/MoveTracker.hpp" #include "util/Repeat.hpp" #include "util/async/Concepts.hpp" -#include "util/async/Error.hpp" #include "util/async/Outcome.hpp" #include "util/async/context/impl/Cancellation.hpp" #include "util/async/context/impl/Timer.hpp" @@ -36,7 +36,6 @@ #include #include #include -#include namespace util::async { namespace impl { @@ -71,7 +70,7 @@ class BasicOperation { }; template -struct BasicScheduledOperation { +struct BasicScheduledOperation : util::MoveTracker { class State { std::mutex m_; std::condition_variable ready_; @@ -105,6 +104,19 @@ struct BasicScheduledOperation { { } + ~BasicScheduledOperation() + { + if (not wasMoved()) + abort(); + } + + BasicScheduledOperation(BasicScheduledOperation const&) = default; + BasicScheduledOperation& + operator=(BasicScheduledOperation const&) = default; + BasicScheduledOperation(BasicScheduledOperation&&) = default; + BasicScheduledOperation& + operator=(BasicScheduledOperation&&) = default; + [[nodiscard]] auto get() { @@ -149,7 +161,8 @@ struct BasicScheduledOperation { * @tparam StopSourceType The type of the stop source */ template -class StoppableOperation : public impl::BasicOperation> { +class StoppableOperation : public impl::BasicOperation>, + public util::MoveTracker { using OutcomeType = StoppableOutcome; StopSourceType stopSource_; @@ -165,6 +178,19 @@ class StoppableOperation : public impl::BasicOperation; * @tparam CtxType The type of the execution context */ template -class RepeatingOperation { +class RepeatingOperation : public util::MoveTracker { util::Repeat repeat_; public: @@ -217,6 +243,12 @@ class RepeatingOperation { repeat_.start(interval, std::forward(fn)); } + ~RepeatingOperation() + { + if (not wasMoved()) + abort(); + } + RepeatingOperation(RepeatingOperation const&) = delete; RepeatingOperation& operator=(RepeatingOperation const&) = delete; diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index f1eda9b2b..c1371f594 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -114,8 +114,6 @@ target_sources( rpc/RPCHelpersTests.cpp rpc/WorkQueueTests.cpp test_data/SslCert.cpp - util/AccountUtilsTests.cpp - util/AssertTests.cpp # Async framework util/async/AnyExecutionContextTests.cpp util/async/AnyOperationTests.cpp @@ -140,6 +138,10 @@ target_sources( util/requests/RequestBuilderTests.cpp util/requests/SslContextTests.cpp util/requests/WsConnectionTests.cpp + # Common utils + util/AccountUtilsTests.cpp + util/AssertTests.cpp + util/MoveTrackerTests.cpp util/RandomTests.cpp util/RetryTests.cpp util/RepeatTests.cpp diff --git a/tests/unit/util/MoveTrackerTests.cpp b/tests/unit/util/MoveTrackerTests.cpp new file mode 100644 index 000000000..366f5a49a --- /dev/null +++ b/tests/unit/util/MoveTrackerTests.cpp @@ -0,0 +1,68 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2025, the clio developers. + + Permission to use, copy, modify, and distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include "util/MoveTracker.hpp" + +#include + +#include + +namespace { +struct MoveMe : util::MoveTracker { + using MoveTracker::wasMoved; // expose as public for tests +}; +} // namespace + +TEST(MoveTrackerTests, SimpleChecks) +{ + auto moveMe = MoveMe(); + EXPECT_FALSE(moveMe.wasMoved()); + + auto other = std::move(moveMe); + EXPECT_TRUE(moveMe.wasMoved()); + EXPECT_FALSE(other.wasMoved()); +} + +TEST(MoveTrackerTests, SupportReuse) +{ + auto original = MoveMe(); + auto other = std::move(original); + + original = std::move(other); + EXPECT_FALSE(original.wasMoved()); + EXPECT_TRUE(other.wasMoved()); +} + +TEST(MoveTrackerTests, SelfMove) +{ + auto original = MoveMe(); + [&](MoveMe& from) { original = std::move(from); }(original); // avoids the compiler catching self-move + + EXPECT_FALSE(original.wasMoved()); +} + +TEST(MoveTrackerTests, SelfMoveAfterWasMoved) +{ + auto original = MoveMe(); + [[maybe_unused]] auto fake = std::move(original); + + [&](MoveMe& from) { original = std::move(from); }(original); // avoids the compiler catching self-move + + EXPECT_TRUE(original.wasMoved()); +} diff --git a/tests/unit/util/async/AsyncExecutionContextTests.cpp b/tests/unit/util/async/AsyncExecutionContextTests.cpp index 04ba5fb94..c78ce115f 100644 --- a/tests/unit/util/async/AsyncExecutionContextTests.cpp +++ b/tests/unit/util/async/AsyncExecutionContextTests.cpp @@ -34,8 +34,6 @@ using namespace util::async; using ::testing::Types; -using ExecutionContextTypes = Types; - template struct ExecutionContextTests : public ::testing::Test { using ExecutionContextType = T; @@ -48,7 +46,15 @@ struct ExecutionContextTests : public ::testing::Test { } }; +// Suite for tests to be ran against all context types but SyncExecutionContext +template +using AsyncExecutionContextTests = ExecutionContextTests; + +using ExecutionContextTypes = Types; +using AsyncExecutionContextTypes = Types; + TYPED_TEST_CASE(ExecutionContextTests, ExecutionContextTypes); +TYPED_TEST_CASE(AsyncExecutionContextTests, AsyncExecutionContextTypes); TYPED_TEST(ExecutionContextTests, move) { @@ -149,6 +155,26 @@ TYPED_TEST(ExecutionContextTests, timerCancel) EXPECT_EQ(value, 42); } +TYPED_TEST(ExecutionContextTests, timerAutoCancels) +{ + auto value = 0; + std::binary_semaphore sem{0}; + { + auto res = this->ctx.scheduleAfter( + std::chrono::milliseconds(1), + [&value, &sem]([[maybe_unused]] auto stopRequested, auto cancelled) { + if (cancelled) + value = 42; + + sem.release(); + } + ); + } // res goes out of scope and cancels the timer + + sem.acquire(); + EXPECT_EQ(value, 42); +} + TYPED_TEST(ExecutionContextTests, timerStdException) { auto res = @@ -247,6 +273,46 @@ TYPED_TEST(ExecutionContextTests, strandWithTimeout) EXPECT_EQ(res.get().value(), 42); } +TYPED_TEST(AsyncExecutionContextTests, executeAutoAborts) +{ + auto value = 0; + std::binary_semaphore sem{0}; + + { + auto res = this->ctx.execute([&](auto stopRequested) { + while (not stopRequested) + ; + value = 42; + sem.release(); + }); + } // res goes out of scope and aborts operation + + sem.acquire(); + EXPECT_EQ(value, 42); +} + +TYPED_TEST(AsyncExecutionContextTests, repeatingOperationAutoAborts) +{ + auto const repeatDelay = std::chrono::milliseconds{1}; + auto const timeout = std::chrono::milliseconds{15}; + auto callCount = 0uz; + auto timeSpentMs = 0u; + + { + auto res = this->ctx.executeRepeatedly(repeatDelay, [&] { ++callCount; }); + timeSpentMs = util::timed([timeout] { std::this_thread::sleep_for(timeout); }); // calculate actual time spent + } // res goes out of scope and automatically aborts the repeating operation + + // double the delay so that if abort did not happen we will fail below expectations + std::this_thread::sleep_for(timeout); + + auto const expectedPureCalls = timeout.count() / repeatDelay.count(); + auto const expectedActualCount = timeSpentMs / repeatDelay.count(); + + EXPECT_GE(callCount, expectedPureCalls / 2u); // expect at least half of the scheduled calls + EXPECT_LE(callCount, expectedActualCount); // never should be called more times than possible before timeout +} + using NoErrorHandlerSyncExecutionContext = BasicExecutionContext< impl::SameThreadContext, impl::BasicStopSource,