Repository: bitcoin/bitcoin
File: src/logging.h (C/C++)

Code before:
            uint64_t dropped_bytes{0};

            Stats(uint64_t max_bytes) : available_bytes{max_bytes} {}
            //! Updates internal accounting and returns true if enough available_bytes were remaining
            bool Consume(uint64_t bytes);
        };

    private:
        mutable StdMutex m_mutex;

        //! Stats for each source location that has attempted to log something.
        std::unordered_map<std::source_location, Stats, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_mutex);
        //! Whether any log locations are suppressed. Cached view on m_source_locations for performance reasons.
        std::atomic<bool> m_suppression_active{false};

    public:
        using SchedulerFunction = std::function<void(std::function<void()>, std::chrono::milliseconds)>;
        /**
         * @param scheduler_func    Callable object used to schedule resetting the window. The first
         *                          parameter is the function to be executed, and the second is the
         *                          reset_window interval.
         * @param max_bytes         Maximum number of bytes that can be logged for each source
         *                          location.
         * @param reset_window      Time window after which the stats are reset.
         */
        LogRateLimiter(SchedulerFunction scheduler_func, uint64_t max_bytes, std::chrono::seconds reset_window);
        //! Maximum number of bytes logged per location per window.
        const uint64_t m_max_bytes;
        //! Interval after which the window is reset.
        const std::chrono::seconds m_reset_window;
        //! Suppression status of a source log location.
        enum class Status {
            UNSUPPRESSED,     // string fits within the limit
            NEWLY_SUPPRESSED, // suppression has started since this string
            STILL_SUPPRESSED, // suppression is still ongoing
        };
        //! Consumes `source_loc`'s available bytes corresponding to the size of the (formatted)
        //! `str` and returns its status.
        [[nodiscard]] Status Consume(
            const std::source_location& source_loc,
            const std::string& str) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
        //! Resets all usage to zero. Called periodically by the scheduler.
        void Reset() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
        //! Returns true if any log locations are currently being suppressed.
        bool SuppressionsActive() const { return m_suppression_active; }
    };

    class Logger
    {
    public:
        struct BufferedLog {

Review comment (performance):
There are some lifetime concerns wrt `scheduler_func`, that are okay in our current codebase, but make the contract perhaps more fragile than it needs to be. Specifically: `Logger` has a `std::unique_ptr<LogRateLimiter> m_limiter` member. The node's scheduler holds [a reference](https://github.com/bitcoin/bitcoin/blob/eb137184482cea8a2a7cdfcd3c73ec6ae5fdd0d6/src/logging.cpp#L379) to the limiter. This means that the limiter may not be destroyed before the scheduler is stopped. In our current code, this is fine, because the logger is a [static global](https://github.com/bitcoin/bitcoin/blob/eb137184482cea8a2a7cdfcd3c73ec6ae5fdd0d6/src/logging.cpp#L43), so it can never be destroyed before the node is shutdown (which stops the scheduler).

I ran into this dependency while reviewing / drafting my suggestion on #32604 when I was writing the test code, when I realized that writing an RAII struct (as now e.g. implemented [here](https://github.com/stickies-v/bitcoin/commit/8aa2726f1cfbc1e4267a796cb000c4eeae5910f5#diff-8c9b223f6f7e311ef2a0585813c17e4cc78119d39ef9956431164336e9158832R285-R287)) for the limiter was not really possible using the `std::unique_ptr` approach. I kept it as-is because the non-test code was fine, and I didn't want to overcomplicate things, but perhaps this follow-up is a good place to at least reconsider this.

Conceptually, I think it makes sense to make `m_limiter` a `std::shared_ptr` since it has two users: the thing that uses it to consume logs (in our case `Logger`), and the thing that uses it to reset stats (in our case `CScheduler`). With a shared pointer, we can safely and explicitly manage these dependencies without introducing coupling between the logger and the scheduler.

After playing around with it, it turns out the implementation is actually rather straightforward:

<details>
<summary>git diff on d0b3a80b7f</summary>

```diff
diff --git a/src/init.cpp b/src/init.cpp
index 63244c802e..75db37397c 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1379,7 +1379,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         }
     }, std::chrono::minutes{5});
 
-    LogInstance().SetRateLimiting(std::make_unique<BCLog::LogRateLimiter>(
+    LogInstance().SetRateLimiting(BCLog::LogRateLimiter::Create(
         [&scheduler](auto func, auto window) { scheduler.scheduleEvery(std::move(func), window); },
         BCLog::RATELIMIT_MAX_BYTES,
         1h));
diff --git a/src/logging.cpp b/src/logging.cpp
index e2c27ec54d..3d42bd9bc8 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -371,12 +371,21 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
            memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
 }
 
-BCLog::LogRateLimiter::LogRateLimiter(
-    SchedulerFunction scheduler_func,
-    uint64_t max_bytes,
-    std::chrono::seconds reset_window) : m_max_bytes{max_bytes}, m_reset_window{reset_window}
+BCLog::LogRateLimiter::LogRateLimiter(uint64_t max_bytes, std::chrono::seconds reset_window)
+    : m_max_bytes{max_bytes}, m_reset_window{reset_window} {}
+
+std::shared_ptr<BCLog::LogRateLimiter> BCLog::LogRateLimiter::Create(
+    SchedulerFunction scheduler_func, uint64_t max_bytes, std::chrono::seconds reset_window)
 {
-    scheduler_func([this] { Reset(); }, reset_window);
+    auto limiter{std::shared_ptr<LogRateLimiter>(new LogRateLimiter(max_bytes, reset_window))};
+    std::weak_ptr<LogRateLimiter> weak_limiter{limiter};
+    scheduler_func([weak_limiter] {
+        if (auto shared_limiter{weak_limiter.lock()}) {
+            shared_limiter->Reset();
+        }
+    },
+                   limiter->m_reset_window);
+    return limiter;
 }
 
 BCLog::LogRateLimiter::Status BCLog::LogRateLimiter::Consume(
diff --git a/src/logging.h b/src/logging.h
index 5437d73c78..12905f1e47 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -18,6 +18,7 @@
 #include <cstring>
 #include <functional>
 #include <list>
+#include <memory>
 #include <mutex>
 #include <source_location>
 #include <string>
@@ -107,7 +108,7 @@ namespace BCLog {
     constexpr uint64_t RATELIMIT_MAX_BYTES{1024 * 1024}; // maximum number of bytes per source location that can be logged within the specified time-window
 
     //! Fixed window rate limiter for logging.
-    class LogRateLimiter
+    class LogRateLimiter : public std::enable_shared_from_this<LogRateLimiter>
     {
     public:
         //! Keeps track of an individual source location and how many available bytes are left for logging from it.
@@ -129,6 +130,7 @@ namespace BCLog {
         std::unordered_map<std::source_location, Stats, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_mutex);
         //! Whether any log locations are suppressed. Cached view on m_source_locations for performance reasons.
         std::atomic<bool> m_suppression_active{false};
+        LogRateLimiter(uint64_t max_bytes, std::chrono::seconds reset_window);
 
     public:
         using SchedulerFunction = std::function<void(std::function<void()>, std::chrono::milliseconds)>;
@@ -140,7 +142,10 @@ namespace BCLog {
          *                          location.
          * @param reset_window      Time window after which the stats are reset.
          */
-        LogRateLimiter(SchedulerFunction scheduler_func, uint64_t max_bytes, std::chrono::seconds reset_window);
+        static std::shared_ptr<LogRateLimiter> Create(
+            SchedulerFunction scheduler_func,
+            uint64_t max_bytes,
+            std::chrono::seconds reset_window);
         //! Maximum number of bytes logged per location per window.
         const uint64_t m_max_bytes;
         //! Interval after which the window is reset.
@@ -160,6 +165,8 @@ namespace BCLog {
         void Reset() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
         //! Returns true if any log locations are currently being suppressed.
         bool SuppressionsActive() const { return m_suppression_active; }
+        //! The function that is used to schedule the reset window.
+        SchedulerFunction m_scheduler_func;
     };
 
     class Logger
@@ -185,7 +192,7 @@ namespace BCLog {
         size_t m_buffer_lines_discarded GUARDED_BY(m_cs){0};
 
         //! Manages the rate limiting of each log location.
-        std::unique_ptr<LogRateLimiter> m_limiter GUARDED_BY(m_cs);
+        std::shared_ptr<LogRateLimiter> m_limiter GUARDED_BY(m_cs);
 
         //! Category-specific log level. Overrides `m_log_level`.
         std::unordered_map<LogFlags, Level> m_category_log_levels GUARDED_BY(m_cs);
@@ -254,7 +261,7 @@ namespace BCLog {
         /** Only for testing */
         void DisconnectTestLogger() EXCLUSIVE_LOCKS_REQUIRED(!m_cs);
 
-        void SetRateLimiting(std::unique_ptr<LogRateLimiter>&& limiter) EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
+        void SetRateLimiting(std::shared_ptr<LogRateLimiter> limiter) EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
         {
             StdLockGuard scoped_lock(m_cs);
             m_limiter = std::move(limiter);
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 81c0bacba0..ea0297367d 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -311,7 +311,8 @@ BOOST_AUTO_TEST_CASE(logging_log_rate_limiter)
     BCLog::LogRateLimiter::SchedulerFunction sched_func{[&scheduler](std::function<void()> func, std::chrono::milliseconds window) {
         scheduler.scheduleEvery(std::move(func), window);
     }};
-    BCLog::LogRateLimiter limiter{sched_func, max_bytes, reset_window};
+    auto limiter_{BCLog::LogRateLimiter::Create(sched_func, max_bytes, reset_window)};
+    auto& limiter{*limiter_};
 
     using Status = BCLog::LogRateLimiter::Status;
     std::source_location source_loc_1{std::source_location::current()};
@@ -427,8 +428,7 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
     BCLog::LogRateLimiter::SchedulerFunction sched_func{[&scheduler](std::function<void()> func, std::chrono::milliseconds window) {
         scheduler.scheduleEvery(std::move(func), window);
     }};
-    std::unique_ptr<BCLog::LogRateLimiter> limiter{std::make_unique<BCLog::LogRateLimiter>(sched_func, bytes_quota, time_window)};
-    LogInstance().SetRateLimiting(std::move(limiter));
+    LogInstance().SetRateLimiting(BCLog::LogRateLimiter::Create(sched_func, bytes_quota, time_window));
 
     std::string log_message(line_length, 'a');
 

```
</details>


What do you think?

Code after:
            //! Remaining bytes
            uint64_t m_available_bytes;
            //! Number of bytes that were consumed but didn't fit in the available bytes.
            uint64_t m_dropped_bytes{0};

            Stats(uint64_t max_bytes) : m_available_bytes{max_bytes} {}
            //! Updates internal accounting and returns true if enough available_bytes were remaining
            bool Consume(uint64_t bytes);
        };

    private:
        mutable StdMutex m_mutex;

        //! Stats for each source location that has attempted to log something.
        std::unordered_map<std::source_location, Stats, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_mutex);
        //! Whether any log locations are suppressed. Cached view on m_source_locations for performance reasons.
        std::atomic<bool> m_suppression_active{false};
        LogRateLimiter(uint64_t max_bytes, std::chrono::seconds reset_window);

    public:
        using SchedulerFunction = std::function<void(std::function<void()>, std::chrono::milliseconds)>;
        /**
         * @param scheduler_func    Callable object used to schedule resetting the window. The first
         *                          parameter is the function to be executed, and the second is the
         *                          reset_window interval.
         * @param max_bytes         Maximum number of bytes that can be logged for each source
         *                          location.
         * @param reset_window      Time window after which the stats are reset.
         */
        static std::shared_ptr<LogRateLimiter> Create(
            SchedulerFunction&& scheduler_func,
            uint64_t max_bytes,
            std::chrono::seconds reset_window);
        //! Maximum number of bytes logged per location per window.
        const uint64_t m_max_bytes;
        //! Interval after which the window is reset.
        const std::chrono::seconds m_reset_window;
        //! Suppression status of a source log location.
        enum class Status {
            UNSUPPRESSED,     // string fits within the limit
            NEWLY_SUPPRESSED, // suppression has started since this string
            STILL_SUPPRESSED, // suppression is still ongoing
        };
        //! Consumes `source_loc`'s available bytes corresponding to the size of the (formatted)
        //! `str` and returns its status.
        [[nodiscard]] Status Consume(
            const std::source_location& source_loc,
            const std::string& str) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
        //! Resets all usage to zero. Called periodically by the scheduler.
        void Reset() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
        //! Returns true if any log locations are currently being suppressed.