From bca23e3e6a85603b8e7a750b0c8bfcf086b911ae Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Fri, 11 Sep 2026 10:02:34 +1000 Subject: [PATCH 1/2] Let a logger be taken back off add_logger built a sink around the caller's callback and discarded the only reference to it, so the callback could never be retired: clear_loggers drops every logger in the process, including ones this caller does not own. A consumer whose callback captures something shorter-lived than the process is then left with a registered callback pointing at freed memory. That is what session-app hit - a bridge that can be closed and reopened, logging from threads its teardown cannot join - and it had to hold the bridge weakly to work around it. add_logger now returns a handle and remove_logger takes it. The handle is a shared_ptr to a forward-declared formatted_callback_sink: spdlog stays out of the public header, and the handle still names a type, so an unrelated shared_ptr cannot be passed as one. Logging is serialised against removal, so once remove_logger returns the callback is neither running nor reachable. The C API is unchanged; it never exposed removal and adding it needs an opaque handle of its own. --- include/session/logging.hpp | 30 ++++++++++++++++++++++++++++-- src/logging.cpp | 17 +++++++++++++---- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/include/session/logging.hpp b/include/session/logging.hpp index 004999019..542190d24 100644 --- a/include/session/logging.hpp +++ b/include/session/logging.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include @@ -11,6 +12,10 @@ namespace spdlog::level { enum level_enum : int; } +namespace oxen::log { +class formatted_callback_sink; +} // namespace oxen::log + namespace session { // This is working roughly like an enum class, but with some useful conversions and comparisons @@ -45,6 +50,11 @@ inline const LogLevel LogLevel::warn{LOG_LEVEL_WARN}; inline const LogLevel LogLevel::error{LOG_LEVEL_ERROR}; inline const LogLevel LogLevel::critical{LOG_LEVEL_CRITICAL}; +/// A registered logger, as returned by `add_logger` and accepted by `remove_logger`. The sink is +/// only forward-declared, so spdlog stays out of this header: hold the handle, don't dereference +/// it. +using LoggerHandle = std::shared_ptr; + /// API: add_logger /// /// Adds a logger callback for oxen-logging log messages (such as from the network object). @@ -56,10 +66,26 @@ inline const LogLevel LogLevel::critical{LOG_LEVEL_CRITICAL}; /// callback(std::string_view msg) /// callback(std::string_view msg, std::string_view log_cat, LogLevel level) /// -void add_logger(std::function cb); -void add_logger( +/// Outputs: +/// - a handle naming this logger, for `remove_logger`. Ignoring it is fine if the logger is +/// meant to last as long as the process. +LoggerHandle add_logger(std::function cb); +LoggerHandle add_logger( std::function cb); +/// API: session/remove_logger +/// +/// Removes a logger added by `add_logger`. Removing one that is not registered does nothing. +/// +/// Logging is serialised against this, so once it returns the callback is neither running nor +/// reachable, and whatever it captured can be destroyed. That is the difference from +/// `clear_loggers`, which drops every logger in the process including ones this caller does not +/// own. +/// +/// Inputs: +/// - `logger` -- [in] the handle returned by `add_logger`. +void remove_logger(const LoggerHandle& logger); + /// API: session/logger_reset_level /// /// Resets the log level of all existing category loggers, and sets a new default for any created diff --git a/src/logging.cpp b/src/logging.cpp index 9c0a24ca6..1be935f29 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -24,12 +24,21 @@ std::string_view LogLevel::to_string() const { return log::to_string(spdlog_level()); } -void add_logger(std::function cb) { - log::add_sink(std::make_shared(std::move(cb))); +LoggerHandle add_logger(std::function cb) { + auto sink = std::make_shared(std::move(cb)); + log::add_sink(sink); + return sink; } -void add_logger( +LoggerHandle add_logger( std::function cb) { - log::add_sink(std::make_shared(std::move(cb))); + auto sink = std::make_shared(std::move(cb)); + log::add_sink(sink); + return sink; +} + +void remove_logger(const LoggerHandle& logger) { + if (logger) + log::remove_sink(logger); } void manual_log(std::string_view msg) { From f07374b36be4ee6abfd50a33697ab1cbbb87f2f1 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Fri, 11 Sep 2026 11:24:15 -0300 Subject: [PATCH 2/2] Bump session-router to bump libquic to bump logging --- external/session-router | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/session-router b/external/session-router index 054d3b9f5..b2508bed1 160000 --- a/external/session-router +++ b/external/session-router @@ -1 +1 @@ -Subproject commit 054d3b9f500e9b6264ead79f8462f88524a142d3 +Subproject commit b2508bed10eb08330598df5bd7fb80415dd34720