From 02c983018459991ec8f7f0d579c7ee3b4ff600ba Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 23 Mar 2026 10:46:16 +0000 Subject: [PATCH] fix: Add Doxygen docs and remove temporary CI filter - Add Doxygen documentation to new methods, members, constants, and macros introduced by the TSAN/ASAN PR (Database::startReadThreads, BasicApp::DeferStart, ResourceManager::start, XRPL_SANITIZER_ACTIVE, coroStackSize, yieldStackSize) - Add @note thread-safety tags where atomics were introduced - Update Database constructor docs to reflect deferred thread startup - Remove temporary CI filter that restricted builds to sanitizer-only variants (must run full matrix before merge) Co-Authored-By: Claude Opus 4.6 --- .github/scripts/strategy-matrix/generate.py | 11 -------- include/xrpl/basics/sanitizers.h | 11 +++++--- include/xrpl/beast/test/yield_to.h | 13 ++++++--- include/xrpl/beast/utility/Journal.h | 2 ++ include/xrpl/core/Coro.ipp | 7 +++-- include/xrpl/nodestore/Database.h | 18 ++++++++++--- include/xrpl/resource/ResourceManager.h | 7 ++++- src/xrpld/app/main/BasicApp.h | 29 ++++++++++++++++++--- 8 files changed, 70 insertions(+), 28 deletions(-) diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index b8868218cb..63eadbdf53 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -324,17 +324,6 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: } ) - # TEMPORARY: Only build previously-failing sanitizer variants to save CI. - # Remove this filter once these configs pass. - sanitizer_only = [ - c - for c in configurations - if c["sanitizers"] - and ("gcc-13" in c["config_name"] or "clang-20" in c["config_name"]) - ] - if sanitizer_only: - return sanitizer_only - return configurations diff --git a/include/xrpl/basics/sanitizers.h b/include/xrpl/basics/sanitizers.h index 8ce50ab45a..8f99fa1478 100644 --- a/include/xrpl/basics/sanitizers.h +++ b/include/xrpl/basics/sanitizers.h @@ -10,9 +10,14 @@ #define XRPL_NO_SANITIZE_ADDRESS #endif -// Detect whether a memory sanitizer (TSAN or ASAN) is active at compile time. -// GCC defines __SANITIZE_THREAD__ / __SANITIZE_ADDRESS__ directly. -// Clang uses __has_feature(thread_sanitizer) / __has_feature(address_sanitizer). +/** Detect whether a memory sanitizer (TSAN or ASAN) is active at compile time. + * + * Evaluates to 1 when the translation unit is compiled with ThreadSanitizer + * or AddressSanitizer instrumentation, 0 otherwise. + * + * GCC defines __SANITIZE_THREAD__ / __SANITIZE_ADDRESS__ directly. + * Clang uses __has_feature(thread_sanitizer) / __has_feature(address_sanitizer). + */ #if defined(__SANITIZE_THREAD__) || defined(__SANITIZE_ADDRESS__) #define XRPL_SANITIZER_ACTIVE 1 #elif defined(__has_feature) diff --git a/include/xrpl/beast/test/yield_to.h b/include/xrpl/beast/test/yield_to.h index 9800a7b400..bbbc0f094a 100644 --- a/include/xrpl/beast/test/yield_to.h +++ b/include/xrpl/beast/test/yield_to.h @@ -19,10 +19,15 @@ namespace beast { namespace test { -// Sanitizers significantly increase stack frame sizes -// (TSAN ~3-5x, ASAN ~2-3x), requiring larger coroutine stacks. -// Note: This duplicates the detection logic from xrpl/basics/sanitizers.h -// because xrpl.beast cannot depend on xrpl.basics (levelization constraint). +/** Stack size for yield_to coroutines. + * + * Sanitizers significantly increase stack frame sizes + * (TSAN ~3-5x, ASAN ~2-3x), requiring larger coroutine stacks. + * + * @note This duplicates the detection logic from xrpl/basics/sanitizers.h + * because xrpl.beast cannot depend on xrpl.basics (levelization + * constraint). + */ #if defined(__SANITIZE_THREAD__) || defined(__SANITIZE_ADDRESS__) inline constexpr std::size_t yieldStackSize = 4 * 1024 * 1024; #elif defined(__has_feature) diff --git a/include/xrpl/beast/utility/Journal.h b/include/xrpl/beast/utility/Journal.h index b2505c00df..9461d7e145 100644 --- a/include/xrpl/beast/utility/Journal.h +++ b/include/xrpl/beast/utility/Journal.h @@ -105,6 +105,8 @@ public: writeAlways(Severity level, std::string const& text) = 0; private: + /// Minimum severity level. Atomic for safe concurrent reads/writes + /// (e.g. RPC threshold changes vs. hot-path log checks). std::atomic thresh_; bool m_console; }; diff --git a/include/xrpl/core/Coro.ipp b/include/xrpl/core/Coro.ipp index 9791c0befd..100eace558 100644 --- a/include/xrpl/core/Coro.ipp +++ b/include/xrpl/core/Coro.ipp @@ -7,8 +7,11 @@ namespace xrpl { -// Sanitizers significantly increase stack frame sizes -// (TSAN ~3-5x, ASAN ~2-3x), requiring larger coroutine stacks. +/** Stack size for JobQueue coroutines. + * + * Sanitizers significantly increase stack frame sizes + * (TSAN ~3-5x, ASAN ~2-3x), requiring larger coroutine stacks. + */ inline constexpr std::size_t coroStackSize = XRPL_SANITIZER_ACTIVE ? megabytes(4) : megabytes(2); template diff --git a/include/xrpl/nodestore/Database.h b/include/xrpl/nodestore/Database.h index 662668910c..204c4f1a0b 100644 --- a/include/xrpl/nodestore/Database.h +++ b/include/xrpl/nodestore/Database.h @@ -34,16 +34,24 @@ public: /** Construct the node store. + Construction configures the database but does not start read threads. + Call startReadThreads() after construction to begin asynchronous reads. + @param scheduler The scheduler to use for performing asynchronous tasks. - @param readThreads The number of asynchronous read threads to create. - @param config The configuration settings + @param readThreads The desired number of asynchronous read threads. + @param config The configuration settings. @param journal Destination for logging output. */ Database(Scheduler& scheduler, int readThreads, Section const& config, beast::Journal j); /** Start the asynchronous read threads. - Must be called after construction to start read threads. - It is safe to destroy the Database without calling this. + + Must be called after construction to start read threads. It is safe + to destroy the Database without calling this; in that case no + asynchronous reads will be serviced. + + @note Not thread-safe. Must be called exactly once, before any + concurrent access to the database. */ void startReadThreads(); @@ -259,6 +267,8 @@ private: std::atomic readStopping_ = false; std::atomic readThreads_ = 0; std::atomic runningThreads_ = 0; + + /// The number of read threads to create when startReadThreads() is called. int const desiredReadThreads_; virtual std::shared_ptr diff --git a/include/xrpl/resource/ResourceManager.h b/include/xrpl/resource/ResourceManager.h index 80e88f88a9..10ad471898 100644 --- a/include/xrpl/resource/ResourceManager.h +++ b/include/xrpl/resource/ResourceManager.h @@ -23,7 +23,12 @@ public: virtual ~Manager() = 0; /** Start the manager's background thread. - Must be called after construction. + + Must be called after construction to begin periodic charge decay + and inactive-consumer sweeps. + + @note Not thread-safe. Must be called exactly once, before any + concurrent access to the manager. */ virtual void start() = 0; diff --git a/src/xrpld/app/main/BasicApp.h b/src/xrpld/app/main/BasicApp.h index b0cc8761b6..6e92181a54 100644 --- a/src/xrpld/app/main/BasicApp.h +++ b/src/xrpld/app/main/BasicApp.h @@ -6,13 +6,22 @@ #include #include -// This is so that the io_context can outlive all the children +/** Manages an io_context and its worker threads. + * + * Ensures the io_context outlives all derived classes by joining worker + * threads in the destructor. Supports immediate or deferred thread startup. + * + * @note Thread-safe after construction completes. The deferred-start + * constructor and startIOThreads() must be called from a single thread. + */ class BasicApp { private: std::optional> work_; std::vector threads_; boost::asio::io_context io_context_; + + /// Number of IO worker threads to create. std::size_t numberOfThreads_; public: @@ -32,13 +41,27 @@ public: } protected: - // Construct without starting threads. Derived classes must call - // startIOThreads() once construction is complete. + /** Tag type for deferred thread startup. + * + * Pass to the protected constructor to construct without starting IO + * threads. The derived class must call startIOThreads() once its own + * construction is complete. + */ struct DeferStart { }; + + /** Construct without starting IO threads. + * + * @param numberOfThreads Desired number of IO worker threads. + * @param Tag to select deferred startup. + */ BasicApp(std::size_t numberOfThreads, DeferStart); + /** Start the IO worker threads. + * + * @note Must be called exactly once after the deferred-start constructor. + */ void startIOThreads(); };