Merge branch 'develop' into 919_Fix_bugs_in_new_webserver

src/app/ClioApplication.cpp

@@ -27,7 +27,6 @@
 #include "etl/NetworkValidatedLedgers.hpp"
 #include "feed/SubscriptionManager.hpp"
 #include "rpc/Counters.hpp"
-#include "rpc/Errors.hpp"
 #include "rpc/RPCEngine.hpp"
 #include "rpc/WorkQueue.hpp"
 #include "rpc/common/impl/HandlerProvider.hpp"
@@ -38,25 +37,16 @@
 #include "web/AdminVerificationStrategy.hpp"
 #include "web/RPCServerHandler.hpp"
 #include "web/Server.hpp"
-#include "web/SubscriptionContextInterface.hpp"
 #include "web/dosguard/DOSGuard.hpp"
 #include "web/dosguard/IntervalSweepHandler.hpp"
 #include "web/dosguard/WhitelistHandler.hpp"
-#include "web/ng/Connection.hpp"
 #include "web/ng/RPCServerHandler.hpp"
-#include "web/ng/Request.hpp"
-#include "web/ng/Response.hpp"
 #include "web/ng/Server.hpp"
 
 #include <boost/asio/io_context.hpp>
-#include <boost/asio/spawn.hpp>
-#include <boost/beast/http/status.hpp>
-#include <boost/json/array.hpp>
-#include <boost/json/parse.hpp>
 
 #include <cstdint>
 #include <cstdlib>
-#include <exception>
 #include <memory>
 #include <optional>
 #include <thread>

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

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();

tests/unit/app/WebHandlersTests.cpp

@@ -42,7 +42,6 @@
 #include <gtest/gtest.h>
 
 #include <memory>
-#include <optional>
 #include <stdexcept>
 #include <string>
 #include <string_view>

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.*"
+    );
+}

tests/unit/web/ng/ResponseTests.cpp

@@ -133,8 +133,8 @@ TEST_F(ResponseTest, asConstBufferJson)
 
 TEST_F(ResponseTest, createFromStringAndConnection)
 {
-    util::TagDecoratorFactory tagDecoratorFactory{util::Config{}};
-    StrictMockConnection connection{"some ip", boost::beast::flat_buffer{}, tagDecoratorFactory};
+    util::TagDecoratorFactory const tagDecoratorFactory{util::Config{}};
+    StrictMockConnection const connection{"some ip", boost::beast::flat_buffer{}, tagDecoratorFactory};
     std::string const responseMessage = "response message";
 
     EXPECT_CALL(connection, wasUpgraded()).WillOnce(testing::Return(false));
@@ -150,8 +150,8 @@ TEST_F(ResponseTest, createFromStringAndConnection)
 
 TEST_F(ResponseTest, createFromJsonAndConnection)
 {
-    util::TagDecoratorFactory tagDecoratorFactory{util::Config{}};
-    StrictMockConnection connection{"some ip", boost::beast::flat_buffer{}, tagDecoratorFactory};
+    util::TagDecoratorFactory const tagDecoratorFactory{util::Config{}};
+    StrictMockConnection const connection{"some ip", boost::beast::flat_buffer{}, tagDecoratorFactory};
     boost::json::object const responseMessage{{"key", "value"}};
 
     EXPECT_CALL(connection, wasUpgraded()).WillOnce(testing::Return(false));