fix: Drop dynamic loggers to fix memory leak (#2686)

src/util/config/ConfigConstraints.hpp

@@ -45,7 +45,7 @@ class ConfigValue;
 /**
  * @brief specific values that are accepted for logger levels in config.
  */
-static constexpr std::array<char const*, 6> kLOG_LEVELS = {
+static constexpr std::array<std::string_view, 6> kLOG_LEVELS = {
     "trace",
     "debug",
     "info",
@@ -57,7 +57,7 @@ static constexpr std::array<char const*, 6> kLOG_LEVELS = {
 /**
  * @brief specific values that are accepted for logger tag style in config.
  */
-static constexpr std::array<char const*, 5> kLOG_TAGS = {
+static constexpr std::array<std::string_view, 5> kLOG_TAGS = {
     "int",
     "uint",
     "null",
@@ -68,7 +68,7 @@ static constexpr std::array<char const*, 5> kLOG_TAGS = {
 /**
  * @brief specific values that are accepted for cache loading in config.
  */
-static constexpr std::array<char const*, 3> kLOAD_CACHE_MODE = {
+static constexpr std::array<std::string_view, 3> kLOAD_CACHE_MODE = {
     "sync",
     "async",
     "none",
@@ -77,17 +77,17 @@ static constexpr std::array<char const*, 3> kLOAD_CACHE_MODE = {
 /**
  * @brief specific values that are accepted for database type in config.
  */
-static constexpr std::array<char const*, 1> kDATABASE_TYPE = {"cassandra"};
+static constexpr std::array<std::string_view, 1> kDATABASE_TYPE = {"cassandra"};
 
 /**
  * @brief specific values that are accepted for server's processing_policy in config.
  */
-static constexpr std::array<char const*, 2> kPROCESSING_POLICY = {"parallel", "sequent"};
+static constexpr std::array<std::string_view, 2> kPROCESSING_POLICY = {"parallel", "sequent"};
 
 /**
  * @brief specific values that are accepted for database provider in config.
  */
-static constexpr std::array<char const*, 2> kPROVIDER = {"cassandra", "aws_keyspace"};
+static constexpr std::array<std::string_view, 2> kPROVIDER = {"cassandra", "aws_keyspace"};
 
 /**
  * @brief An interface to enforce constraints on certain values within ClioConfigDefinition.
@@ -123,7 +123,7 @@ protected:
      */
     template <std::size_t ArrSize>
     constexpr std::string
-    makeErrorMsg(std::string_view key, Value const& value, std::array<char const*, ArrSize> arr) const
+    makeErrorMsg(std::string_view key, Value const& value, std::array<std::string_view, ArrSize> arr) const
     {
         // Extract the value from the variant
         auto const valueStr = std::visit([](auto const& v) { return fmt::format("{}", v); }, value);
@@ -271,7 +271,7 @@ public:
      * @param key The key of the ConfigValue that has this constraint
      * @param arr The value that has this constraint must be of the values in arr
      */
-    constexpr OneOf(std::string_view key, std::array<char const*, ArrSize> arr) : key_{key}, arr_{arr}
+    constexpr OneOf(std::string_view key, std::array<std::string_view, ArrSize> arr) : key_{key}, arr_{arr}
     {
     }
 
@@ -318,7 +318,7 @@ private:
     print(std::ostream& stream) const override
     {
         std::string valuesStream;
-        std::ranges::for_each(arr_, [&valuesStream](std::string const& elem) {
+        std::ranges::for_each(arr_, [&valuesStream](std::string_view elem) {
             valuesStream += fmt::format(" `{}`,", elem);
         });
         // replace the last "," with "."
@@ -327,7 +327,7 @@ private:
     }
 
     std::string_view key_;
-    std::array<char const*, ArrSize> arr_;
+    std::array<std::string_view, ArrSize> arr_;
 };
 
 /**

src/util/log/Logger.cpp

@@ -220,10 +220,10 @@ LogService::createFileSink(FileLoggingParams const& params, std::string const& f
  * @param defaultSeverity The default severity level to use if not overridden.
  * @return A map of channel names to their minimum severity levels, or an error message if parsing fails.
  */
-static std::expected<std::unordered_map<std::string, Severity>, std::string>
+static std::expected<std::unordered_map<std::string_view, Severity>, std::string>
 getMinSeverity(config::ClioConfigDefinition const& config, Severity defaultSeverity)
 {
-    std::unordered_map<std::string, Severity> minSeverity;
+    std::unordered_map<std::string_view, Severity> minSeverity;
     for (auto const& channel : Logger::kCHANNELS)
         minSeverity[channel] = defaultSeverity;
 
@@ -284,13 +284,15 @@ LogServiceState::reset()
 }
 
 std::shared_ptr<spdlog::logger>
-LogServiceState::registerLogger(std::string const& channel, std::optional<Severity> severity)
+LogServiceState::registerLogger(std::string_view channel, std::optional<Severity> severity)
 {
     if (not initialized_) {
         throw std::logic_error("LogService is not initialized");
     }
 
-    std::shared_ptr<spdlog::logger> existingLogger = spdlog::get(channel);
+    std::string const channelStr{channel};
+
+    std::shared_ptr<spdlog::logger> existingLogger = spdlog::get(channelStr);
     if (existingLogger != nullptr) {
         if (severity.has_value())
             existingLogger->set_level(toSpdlogLevel(*severity));
@@ -300,10 +302,10 @@ LogServiceState::registerLogger(std::string const& channel, std::optional<Severi
     std::shared_ptr<spdlog::logger> logger;
     if (isAsync_) {
         logger = std::make_shared<spdlog::async_logger>(
-            channel, sinks_.begin(), sinks_.end(), spdlog::thread_pool(), spdlog::async_overflow_policy::block
+            channelStr, sinks_.begin(), sinks_.end(), spdlog::thread_pool(), spdlog::async_overflow_policy::block
         );
     } else {
-        logger = std::make_shared<spdlog::logger>(channel, sinks_.begin(), sinks_.end());
+        logger = std::make_shared<spdlog::logger>(channelStr, sinks_.begin(), sinks_.end());
     }
 
     logger->set_level(toSpdlogLevel(severity.value_or(defaultSeverity_)));
@@ -427,10 +429,25 @@ LogServiceState::replaceSinks(std::vector<std::shared_ptr<spdlog::sinks::sink>>
     spdlog::apply_all([](std::shared_ptr<spdlog::logger> logger) { logger->sinks() = sinks_; });
 }
 
-Logger::Logger(std::string channel) : logger_(LogServiceState::registerLogger(channel))
+Logger::Logger(std::string_view const channel) : logger_(LogServiceState::registerLogger(channel))
 {
 }
 
+Logger::~Logger()
+{
+    // One reference is held by logger_ and the other by spdlog registry
+    static constexpr size_t kLAST_LOGGER_REF_COUNT = 2;
+
+    if (logger_ == nullptr) {
+        return;
+    }
+
+    bool const isDynamic = !std::ranges::contains(kCHANNELS, logger_->name());
+    if (isDynamic && logger_.use_count() == kLAST_LOGGER_REF_COUNT) {
+        spdlog::drop(logger_->name());
+    }
+}
+
 Logger::Pump::Pump(std::shared_ptr<spdlog::logger> logger, Severity sev, SourceLocationType const& loc)
     : logger_(std::move(logger))
     , severity_(sev)

src/util/log/Logger.hpp

@@ -29,6 +29,7 @@
 #include <optional>
 #include <sstream>
 #include <string>
+#include <string_view>
 #include <vector>
 
 // We forward declare spdlog::logger and spdlog::sinks::sink
@@ -91,7 +92,7 @@ enum class Severity {
  * otherwise. See @ref LogService::init() for setup of the logging core and
  * severity levels for each channel.
  */
-class Logger final {
+class Logger {
     std::shared_ptr<spdlog::logger> logger_;
 
     friend class LogService;  // to expose the Pump interface
@@ -145,7 +146,7 @@ class Logger final {
     };
 
 public:
-    static constexpr std::array<char const*, 8> kCHANNELS = {
+    static constexpr std::array<std::string_view, 8> kCHANNELS = {
         "General",
         "WebServer",
         "Backend",
@@ -165,10 +166,10 @@ public:
      *
      * @param channel The channel this logger will report into.
      */
-    Logger(std::string channel);
+    Logger(std::string_view const channel);
 
     Logger(Logger const&) = default;
-    ~Logger() = default;
+    ~Logger();
 
     Logger(Logger&&) = default;
     Logger&
@@ -291,7 +292,7 @@ protected:
      * @return Shared pointer to the registered spdlog logger
      */
     static std::shared_ptr<spdlog::logger>
-    registerLogger(std::string const& channel, std::optional<Severity> severity = std::nullopt);
+    registerLogger(std::string_view channel, std::optional<Severity> severity = std::nullopt);
 
 protected:
     static bool isAsync_;                                             // NOLINT(readability-identifier-naming)

tests/common/util/LoggerFixtures.cpp

@@ -35,7 +35,7 @@ LoggerFixture::init()
 {
     util::LogServiceState::init(false, util::Severity::FTL, {});
 
-    std::ranges::for_each(util::Logger::kCHANNELS, [](char const* channel) {
+    std::ranges::for_each(util::Logger::kCHANNELS, [](std::string_view const channel) {
         util::LogService::registerLogger(channel);
     });
 

tests/unit/util/config/ConfigValueTests.cpp

@@ -31,6 +31,7 @@
 #include <optional>
 #include <ostream>
 #include <string>
+#include <string_view>
 
 using namespace util::config;
 
@@ -164,7 +165,7 @@ TEST_F(ConstraintTest, SetValuesOnPortConstraint)
 
 TEST_F(ConstraintTest, OneOfConstraintOneValue)
 {
-    std::array<char const*, 1> const arr = {"tracer"};
+    std::array<std::string_view, 1> const arr = {"tracer"};
     auto const databaseConstraint{OneOf{"database.type", arr}};
     EXPECT_FALSE(databaseConstraint.checkConstraint("tracer").has_value());
 
@@ -180,7 +181,7 @@ TEST_F(ConstraintTest, OneOfConstraintOneValue)
 
 TEST_F(ConstraintTest, OneOfConstraint)
 {
-    std::array<char const*, 3> const arr = {"123", "trace", "haha"};
+    std::array<std::string_view, 3> const arr = {"123", "trace", "haha"};
     auto const oneOfCons{OneOf{"log.level", arr}};
 
     EXPECT_FALSE(oneOfCons.checkConstraint("trace").has_value());

tests/unit/util/log/LogServiceInitTests.cpp

@@ -101,7 +101,7 @@ TEST_F(LogServiceInitTests, DefaultLogLevel)
     EXPECT_TRUE(LogService::init(config_));
 
     std::string const logString = "some log";
-    for (auto const& channel : Logger::kCHANNELS) {
+    for (std::string_view const channel : Logger::kCHANNELS) {
         Logger const log{channel};
         log.trace() << logString;
         auto loggerStr = getLoggerString();

tests/unit/util/log/LoggerTests.cpp

@@ -21,11 +21,23 @@
 #include "util/log/Logger.hpp"
 
 #include <gtest/gtest.h>
+#include <spdlog/logger.h>
+#include <spdlog/spdlog.h>
 
 #include <cstddef>
 #include <string>
 using namespace util;
 
+namespace {
+size_t
+loggersNum()
+{
+    size_t counter = 0;
+    spdlog::apply_all([&counter](std::shared_ptr<spdlog::logger>) { ++counter; });
+    return counter;
+}
+}  // namespace
+
 // Used as a fixture for tests with enabled logging
 class LoggerTest : public LoggerFixture {};
 
@@ -71,3 +83,24 @@ TEST_F(LoggerTest, LOGMacro)
     EXPECT_TRUE(computeCalled);
 }
 #endif
+
+TEST_F(LoggerTest, ManyDynamicLoggers)
+{
+    static constexpr size_t kNUM_LOGGERS = 10'000;
+
+    auto initialLoggers = loggersNum();
+
+    for (size_t i = 0; i < kNUM_LOGGERS; ++i) {
+        std::string const loggerName = "DynamicLogger" + std::to_string(i);
+
+        Logger log{loggerName};
+        log.info() << "Logger number " << i;
+        ASSERT_EQ(getLoggerString(), "inf:" + loggerName + " - Logger number " + std::to_string(i) + "\n");
+
+        Logger copy = log;
+        copy.info() << "Copy of logger number " << i;
+        ASSERT_EQ(getLoggerString(), "inf:" + loggerName + " - Copy of logger number " + std::to_string(i) + "\n");
+    }
+
+    ASSERT_EQ(loggersNum(), initialLoggers);
+}