fix: AsyncFramework RAII (#1815)

Fixes #1812

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 <utility>
+
+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

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 <memory>
 #include <mutex>
 #include <optional>
-#include <thread>
 
 namespace util::async {
 namespace impl {
@@ -71,7 +70,7 @@ public:
 };
 
 template <typename CtxType, typename OpType>
-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 <typename RetType, typename StopSourceType>
-class StoppableOperation : public impl::BasicOperation<StoppableOutcome<RetType, StopSourceType>> {
+class StoppableOperation : public impl::BasicOperation<StoppableOutcome<RetType, StopSourceType>>,
+                           public util::MoveTracker {
     using OutcomeType = StoppableOutcome<RetType, StopSourceType>;
 
     StopSourceType stopSource_;
@@ -165,6 +178,19 @@ public:
     {
     }
 
+    ~StoppableOperation()
+    {
+        if (not wasMoved())
+            requestStop();
+    }
+
+    StoppableOperation(StoppableOperation const&) = delete;
+    StoppableOperation&
+    operator=(StoppableOperation const&) = delete;
+    StoppableOperation(StoppableOperation&&) = default;
+    StoppableOperation&
+    operator=(StoppableOperation&&) = default;
+
     /** @brief Requests the operation to stop */
     void
     requestStop() noexcept
@@ -199,7 +225,7 @@ using ScheduledOperation = impl::BasicScheduledOperation<CtxType, OpType>;
  * @tparam CtxType The type of the execution context
  */
 template <typename CtxType>
-class RepeatingOperation {
+class RepeatingOperation : public util::MoveTracker {
     util::Repeat repeat_;
 
 public:
@@ -217,6 +243,12 @@ public:
         repeat_.start(interval, std::forward<decltype(fn)>(fn));
     }
 
+    ~RepeatingOperation()
+    {
+        if (not wasMoved())
+            abort();
+    }
+
     RepeatingOperation(RepeatingOperation const&) = delete;
     RepeatingOperation&
     operator=(RepeatingOperation const&) = delete;

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

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 <gtest/gtest.h>
+
+#include <utility>
+
+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());
+}

tests/unit/util/async/AsyncExecutionContextTests.cpp

@@ -34,8 +34,6 @@
 using namespace util::async;
 using ::testing::Types;
 
-using ExecutionContextTypes = Types<CoroExecutionContext, PoolExecutionContext, SyncExecutionContext>;
-
 template <typename T>
 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 <typename T>
+using AsyncExecutionContextTests = ExecutionContextTests<T>;
+
+using ExecutionContextTypes = Types<CoroExecutionContext, PoolExecutionContext, SyncExecutionContext>;
+using AsyncExecutionContextTypes = Types<CoroExecutionContext, PoolExecutionContext>;
+
 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,