fix: Improve Repeat implementation (#1775)

XRPLF · Dec 11, 2024 · b53cfd0 · b53cfd0
1 parent c41399e
commit b53cfd0
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 14 deletions.
diff --git a/src/util/Repeat.cpp b/src/util/Repeat.cpp
@@ -27,12 +27,16 @@ Repeat::Repeat(boost::asio::io_context& ioc) : timer_(ioc)
 {
 }
 
+Repeat::~Repeat()
+{
+    *stopped_ = true;
+}
+
 void
 Repeat::stop()
 {
-    stopping_ = true;
+    *stopped_ = true;
     timer_.cancel();
-    semaphore_.acquire();
 }
 
 }  // namespace util
diff --git a/src/util/Repeat.hpp b/src/util/Repeat.hpp
@@ -19,14 +19,16 @@
 
 #pragma once
 
+#include "util/Assert.hpp"
+
 #include <boost/asio/io_context.hpp>
 #include <boost/asio/post.hpp>
 #include <boost/asio/steady_timer.hpp>
 
 #include <atomic>
 #include <chrono>
 #include <concepts>
-#include <semaphore>
+#include <memory>
 
 namespace util {
 
@@ -36,8 +38,7 @@ namespace util {
  */
 class Repeat {
     boost::asio::steady_timer timer_;
-    std::atomic_bool stopping_{false};
-    std::binary_semaphore semaphore_{0};
+    std::shared_ptr<std::atomic_bool> stopped_ = std::make_shared<std::atomic_bool>(true);
 
 public:
     /**
@@ -47,6 +48,11 @@ class Repeat {
      */
     Repeat(boost::asio::io_context& ioc);
 
+    /**
+     * @brief Destroy the Repeat object
+     */
+    ~Repeat();
+
     /**
      * @brief Stop repeating
      * @note This method will block to ensure the repeating is actually stopped. But blocking time should be very short.
@@ -56,6 +62,7 @@ class Repeat {
 
     /**
      * @brief Start asynchronously repeating
+     * @note stop() must be called before start() is called for the second time
      *
      * @tparam Action The action type
      * @param interval The interval to repeat
@@ -65,7 +72,9 @@ class Repeat {
     void
     start(std::chrono::steady_clock::duration interval, Action&& action)
     {
-        stopping_ = false;
+        ASSERT(*stopped_, "Repeat should be stopped before the next use");
+        // Create a new variable for each start() to make each start()-stop() session independent
+        stopped_ = std::make_shared<std::atomic_bool>(false);
         startImpl(interval, std::forward<Action>(action));
     }
 
@@ -75,9 +84,10 @@ class Repeat {
     startImpl(std::chrono::steady_clock::duration interval, Action&& action)
     {
         timer_.expires_after(interval);
-        timer_.async_wait([this, interval, action = std::forward<Action>(action)](auto const&) mutable {
-            if (stopping_) {
-                semaphore_.release();
+        timer_.async_wait([this, interval, stopping = stopped_, action = std::forward<Action>(action)](
+                              auto const& errorCode
+                          ) mutable {
+            if (errorCode or *stopping) {
                 return;
             }
             action();

diff --git a/tests/unit/util/RepeatTests.cpp b/tests/unit/util/RepeatTests.cpp
@@ -34,7 +34,7 @@
 using namespace util;
 using testing::AtLeast;
 
-struct RepeatTests : SyncAsioContextTest {
+struct RepeatTest : SyncAsioContextTest {
     Repeat repeat{ctx};
     testing::StrictMock<testing::MockFunction<void()>> handlerMock;
 
@@ -51,14 +51,14 @@ struct RepeatTests : SyncAsioContextTest {
     }
 };
 
-TEST_F(RepeatTests, CallsHandler)
+TEST_F(RepeatTest, CallsHandler)
 {
     repeat.start(std::chrono::milliseconds{1}, handlerMock.AsStdFunction());
     EXPECT_CALL(handlerMock, Call).Times(AtLeast(10));
     runContextFor(std::chrono::milliseconds{20});
 }
 
-TEST_F(RepeatTests, StopsOnStop)
+TEST_F(RepeatTest, StopsOnStop)
 {
     withRunningContext([this]() {
         repeat.start(std::chrono::milliseconds{1}, handlerMock.AsStdFunction());
@@ -68,14 +68,27 @@ TEST_F(RepeatTests, StopsOnStop)
     });
 }
 
-TEST_F(RepeatTests, RunsAfterStop)
+TEST_F(RepeatTest, RunsAfterStop)
 {
     withRunningContext([this]() {
-        for ([[maybe_unused]] auto _ : std::ranges::iota_view(0, 2)) {
+        for ([[maybe_unused]] auto i : std::ranges::iota_view(0, 2)) {
             repeat.start(std::chrono::milliseconds{1}, handlerMock.AsStdFunction());
             EXPECT_CALL(handlerMock, Call).Times(AtLeast(1));
             std::this_thread::sleep_for(std::chrono::milliseconds{10});
             repeat.stop();
         }
     });
 }
+
+struct RepeatDeathTest : RepeatTest {};
+
+TEST_F(RepeatDeathTest, DiesWhenStartCalledTwice)
+{
+    EXPECT_DEATH(
+        {
+            repeat.start(std::chrono::seconds{1}, []() {});
+            repeat.start(std::chrono::seconds{1}, []() {});
+        },
+        "Assertion .* failed.*"
+    );
+}