From 582200a1f8ddd6f435d1264668c7074121c2636c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sat, 23 Aug 2025 00:15:53 +0200 Subject: [PATCH] Make message macros (FAIL, WARN, INFO, etc) thread safe This builds on the existing work to make assertion thread safe, by adding an extra synchronization point in the holder of `ReusableStringStream`'s stream instances, as those are used to build the messages, and finishing the move of message scope holders to be thread-local. --- docs/thread-safety.md | 131 +++++++++++++++--- src/catch2/catch_message.cpp | 3 +- src/catch2/internal/catch_message_info.cpp | 11 +- src/catch2/internal/catch_message_info.hpp | 2 +- .../internal/catch_reusable_string_stream.cpp | 15 +- src/catch2/internal/catch_run_context.cpp | 54 +++++--- src/catch2/internal/catch_run_context.hpp | 3 - 7 files changed, 165 insertions(+), 54 deletions(-) diff --git a/docs/thread-safety.md b/docs/thread-safety.md index c3f610f0..3d48eb0b 100644 --- a/docs/thread-safety.md +++ b/docs/thread-safety.md @@ -2,7 +2,9 @@ # Thread safety in Catch2 **Contents**
-[Using assertion macros from multiple threads](#using-assertion-macros-from-multiple-threads)
+[Using assertion macros from spawned threads](#using-assertion-macros-from-spawned-threads)
+[Assertion-like message macros and spawned threads](#assertion-like-message-macros-and-spawned-threads)
+[Message macros and spawned threads](#message-macros-and-spawned-threads)
[examples](#examples)
[`STATIC_REQUIRE` and `STATIC_CHECK`](#static_require-and-static_check)
[Fatal errors and multiple threads](#fatal-errors-and-multiple-threads)
@@ -10,17 +12,18 @@ > Thread safe assertions were introduced in Catch2 3.9.0 -Thread safety in Catch2 is currently limited to all the assertion macros. -Interacting with benchmark macros, message macros (e.g. `INFO` or `CAPTURE`), -sections macros, generator macros, or test case macros is not thread-safe. -The message macros are likely to be made thread-safe in the future, but -the way sections define test runs is incompatible with user being able -to spawn threads arbitrarily, thus that limitation is here to stay. +Thread safety in Catch2 is currently limited to all the assertion macros, +and to message or message-adjacent macros (e.g. `INFO` or `WARN`). + +Interacting with benchmark macros, sections macros, generator macros, or +test case macros is not thread-safe. The way sections define paths through +the test is incompatible with user spawning threads arbitrarily, so this +limitation is here to stay. **Important: thread safety in Catch2 is [opt-in](configuration.md#experimental-thread-safety)** -## Using assertion macros from multiple threads +## Using assertion macros from spawned threads The full set of Catch2's runtime assertion macros is thread-safe. However, it is important to keep in mind that their semantics might not support @@ -30,7 +33,7 @@ Specifically, the `REQUIRE` family of assertion macros have semantics of stopping the test execution on failure. This is done by throwing an exception, but since the user-spawned thread will not have the test-level try-catch block ready to catch the test failure exception, failing a -`REQUIRE` assertion inside this thread will terminate the process. +`REQUIRE` assertion inside user-spawned thread will terminate the process. The `CHECK` family of assertions does not have this issue, because it does not try to stop the test execution. @@ -38,16 +41,32 @@ does not try to stop the test execution. Note that `CHECKED_IF` and `CHECKED_ELSE` are also thread safe (internally they are assertion macro + an if). -**`SKIP()`, `FAIL()`, `SUCCEED()` are not assertion macros, and are not -thread-safe.** + +## Assertion-like message macros and spawned threads + +Similarly to assertion macros, not all assertion-like message macros can +be used from spawned thread. + +`SKIP` and `FAIL` macros stop the test execution. Just like with `REQUIRE`, +this means that they cannot be used inside user-spawned threads. `SUCCEED`, +`FAIL_CHECK` and `WARN` do not attempt to stop the test execution and +thus can be used from any thread. + + +## Message macros and spawned threads + +Macros that add extra messages to following assertion, such as `INFO` +or `CAPTURE`, are all thread safe and can be used in any thread. Note +that these messages are per-thread, and thus `INFO` inside a user-spawned +thread will not be seen by the main thread, and vice versa. ## examples -### `REQUIRE` from main thread, `CHECK` from spawned threads +### `REQUIRE` from the main thread, `CHECK` from spawned threads ```cpp -TEST_CASE( "Failed REQUIRE in main thread is fine" ) { +TEST_CASE( "Failed REQUIRE in the main thread is fine" ) { std::vector threads; for ( size_t t = 0; t < 16; ++t) { threads.emplace_back( []() { @@ -85,7 +104,7 @@ TEST_CASE( "Successful REQUIRE in spawned thread is fine" ) { This will also work as expected, because the `REQUIRE` is successful. ```cpp -TEST_CASE( "Failed REQUIRE in spawned thread is fine" ) { +TEST_CASE( "Failed REQUIRE in spawned thread kills the process" ) { std::vector threads; for ( size_t t = 0; t < 16; ++t) { threads.emplace_back( []() { @@ -99,12 +118,88 @@ TEST_CASE( "Failed REQUIRE in spawned thread is fine" ) { This will fail catastrophically and terminate the process. +### INFO across threads + +```cpp +TEST_CASE( "messages don't cross threads" ) { + std::jthread t1( [&]() { + for ( size_t i = 0; i < 100; ++i ) { + INFO( "spawned thread #1" ); + CHECK( 1 == 1 ); + } + } ); + + std::thread t2( [&]() { + for (size_t i = 0; i < 100; ++i) { + UNSCOPED_INFO( "spawned thread #2" ); + } + } ); + + for (size_t i = 0; i < 100; ++i) { + CHECK( 1 == 2 ); + } +} +``` +None of the failed checks will show the "spawned thread #1" message, as +that message is for the `t1` thread. If the reporter shows passing +assertions (e.g. due to the tests being run with `-s`), you will see the +"spawned thread #1" message alongside the passing `CHECK( 1 == 1 )` assertion. + +The message "spawned thread #2" will never be shown, because there are no +assertions in `t2`. + + +### FAIL/SKIP from the main thread + +```cpp +TEST_CASE( "FAIL in the main thread is fine" ) { + std::vector threads; + for ( size_t t = 0; t < 16; ++t) { + threads.emplace_back( []() { + for (size_t i = 0; i < 10; ++i) { + CHECK( true ); + CHECK( false ); + } + } ); + } + + FAIL(); +} +``` + +This will work as expected, that is, the process will finish running +normally, the test case will fail and there will be 321 total assertions, +160 passing and 161 failing (`FAIL` counts as failed assertion). + +However, when the main thread hits `FAIL`, it will wait for the other +threads to finish due to `std::jthread`'s destructor joining the spawned +thread. Due to this, using `SKIP` is not recommended once more threads +are spawned; while the main thread will bail from the test execution, +the spawned threads will keep running and may fail the test case. + + +### FAIL/SKIP from spawned threads + +```cpp +TEST_CASE( "FAIL/SKIP in spawned thread kills the process" ) { + std::vector threads; + for ( size_t t = 0; t < 16; ++t) { + threads.emplace_back( []() { + for (size_t i = 0; i < 10'000; ++i) { + FAIL(); + } + } ); + } +} +``` +As with failing `REQUIRE`, both `FAIL` and `SKIP` in spawned threads +terminate the process. + + ## `STATIC_REQUIRE` and `STATIC_CHECK` -None of `STATIC_REQUIRE`, `STATIC_REQUIRE_FALSE`, `STATIC_CHECK`, and -`STATIC_CHECK_FALSE` are currently thread safe. This might be surprising -given that they are a compile-time checks, but they also rely on the -message macros to register the result with reporter at runtime. +All of `STATIC_REQUIRE`, `STATIC_REQUIRE_FALSE`, `STATIC_CHECK`, and +`STATIC_CHECK_FALSE` are thread safe in the delayed evaluation configuration. ## Fatal errors and multiple threads diff --git a/src/catch2/catch_message.cpp b/src/catch2/catch_message.cpp index d3914a80..9cb0167f 100644 --- a/src/catch2/catch_message.cpp +++ b/src/catch2/catch_message.cpp @@ -101,8 +101,9 @@ namespace Catch { } Capturer::~Capturer() { assert( m_captured == m_messages.size() ); - for ( size_t i = 0; i < m_captured; ++i ) + for ( size_t i = 0; i < m_captured; ++i ) { m_resultCapture.popScopedMessage( m_messages[i] ); + } } void Capturer::captureValue( size_t index, std::string const& value ) { diff --git a/src/catch2/internal/catch_message_info.cpp b/src/catch2/internal/catch_message_info.cpp index e0e9dc7e..bc887abf 100644 --- a/src/catch2/internal/catch_message_info.cpp +++ b/src/catch2/internal/catch_message_info.cpp @@ -10,16 +10,17 @@ namespace Catch { - MessageInfo::MessageInfo( StringRef _macroName, - SourceLineInfo const& _lineInfo, - ResultWas::OfType _type ) + MessageInfo::MessageInfo( StringRef _macroName, + SourceLineInfo const& _lineInfo, + ResultWas::OfType _type ) : macroName( _macroName ), lineInfo( _lineInfo ), type( _type ), sequence( ++globalCount ) {} - // This may need protecting if threading support is added - unsigned int MessageInfo::globalCount = 0; + // Messages are owned by their individual threads, so the counter should be thread-local as well. + // Alternative consideration: atomic, so threads don't share IDs and things are easier to debug. + thread_local unsigned int MessageInfo::globalCount = 0; } // end namespace Catch diff --git a/src/catch2/internal/catch_message_info.hpp b/src/catch2/internal/catch_message_info.hpp index ded098f5..bd686d6d 100644 --- a/src/catch2/internal/catch_message_info.hpp +++ b/src/catch2/internal/catch_message_info.hpp @@ -37,7 +37,7 @@ namespace Catch { return sequence < other.sequence; } private: - static unsigned int globalCount; + static thread_local unsigned int globalCount; }; } // end namespace Catch diff --git a/src/catch2/internal/catch_reusable_string_stream.cpp b/src/catch2/internal/catch_reusable_string_stream.cpp index 33eafde4..a2dd5a63 100644 --- a/src/catch2/internal/catch_reusable_string_stream.cpp +++ b/src/catch2/internal/catch_reusable_string_stream.cpp @@ -7,6 +7,7 @@ // SPDX-License-Identifier: BSL-1.0 #include #include +#include #include #include @@ -20,8 +21,10 @@ namespace Catch { std::vector> m_streams; std::vector m_unused; std::ostringstream m_referenceStream; // Used for copy state/ flags from + Detail::Mutex m_mutex; auto add() -> std::size_t { + Detail::LockGuard _( m_mutex ); if( m_unused.empty() ) { m_streams.push_back( Detail::make_unique() ); return m_streams.size()-1; @@ -33,9 +36,13 @@ namespace Catch { } } - void release( std::size_t index ) { - m_streams[index]->copyfmt( m_referenceStream ); // Restore initial flags and other state - m_unused.push_back(index); + void release( std::size_t index, std::ostream* originalPtr ) { + assert( originalPtr ); + originalPtr->copyfmt( m_referenceStream ); // Restore initial flags and other state + + Detail::LockGuard _( m_mutex ); + assert( originalPtr == m_streams[index].get() && "Mismatch between release index and stream ptr" ); + m_unused.push_back( index ); } }; @@ -47,7 +54,7 @@ namespace Catch { ReusableStringStream::~ReusableStringStream() { static_cast( m_oss )->str(""); m_oss->clear(); - Singleton::getMutable().release( m_index ); + Singleton::getMutable().release( m_index, m_oss ); } std::string ReusableStringStream::str() const { diff --git a/src/catch2/internal/catch_run_context.cpp b/src/catch2/internal/catch_run_context.cpp index 15e25322..d1cec82c 100644 --- a/src/catch2/internal/catch_run_context.cpp +++ b/src/catch2/internal/catch_run_context.cpp @@ -172,9 +172,6 @@ namespace Catch { // This also implies that messages are owned by their respective // threads, and should not be shared across different threads. // - // For simplicity, we disallow messages in multi-threaded contexts, - // but in the future we can enable them under this logic. - // // This implies that various pieces of metadata referring to last // assertion result/source location/message handling, etc // should also be thread local. For now we just use naked globals @@ -183,15 +180,27 @@ namespace Catch { // This is used for the "if" part of CHECKED_IF/CHECKED_ELSE static thread_local bool g_lastAssertionPassed = false; - // Should we clear message scopes before sending off the messages to - // reporter? Set in `assertionPassedFastPath` to avoid doing the full - // clear there for performance reasons. - static thread_local bool g_clearMessageScopes = false; + // This is the source location for last encountered macro. It is // used to provide the users with more precise location of error // when an unexpected exception/fatal error happens. static thread_local SourceLineInfo g_lastKnownLineInfo("DummyLocation", static_cast(-1)); - } + + // Should we clear message scopes before sending off the messages to + // reporter? Set in `assertionPassedFastPath` to avoid doing the full + // clear there for performance reasons. + static thread_local bool g_clearMessageScopes = false; + + CATCH_INTERNAL_START_WARNINGS_SUPPRESSION + CATCH_INTERNAL_SUPPRESS_GLOBALS_WARNINGS + // Actual messages to be provided to the reporter + static thread_local std::vector g_messages; + + // Owners for the UNSCOPED_X information macro + static thread_local std::vector g_messageScopes; + CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION + + } // namespace Detail RunContext::RunContext(IConfig const* _config, IEventListenerPtr&& reporter) : m_runInfo(_config->name()), @@ -327,20 +336,21 @@ namespace Catch { Detail::g_lastAssertionPassed = true; } + if ( Detail::g_clearMessageScopes ) { + Detail::g_messageScopes.clear(); + Detail::g_clearMessageScopes = false; + } + // From here, we are touching shared state and need mutex. Detail::LockGuard lock( m_assertionMutex ); { - if ( Detail::g_clearMessageScopes ) { - m_messageScopes.clear(); - Detail::g_clearMessageScopes = false; - } auto _ = scopedDeactivate( *m_outputRedirect ); updateTotalsFromAtomics(); - m_reporter->assertionEnded( AssertionStats( result, m_messages, m_totals ) ); + m_reporter->assertionEnded( AssertionStats( result, Detail::g_messages, m_totals ) ); } if ( result.getResultType() != ResultWas::Warning ) { - m_messageScopes.clear(); + Detail::g_messageScopes.clear(); } // Reset working state. assertion info will be reset after @@ -473,8 +483,8 @@ namespace Catch { m_reporter->benchmarkFailed( error ); } - void RunContext::pushScopedMessage(MessageInfo const & message) { - m_messages.push_back(message); + void RunContext::pushScopedMessage( MessageInfo const& message ) { + Detail::g_messages.push_back( message ); } void RunContext::popScopedMessage( MessageInfo const& message ) { @@ -483,16 +493,16 @@ namespace Catch { // messages than low single digits, so the optimization is tiny, // and we would have to hand-write the loop to avoid terrible // codegen of reverse iterators in debug mode. - m_messages.erase( - std::find_if( m_messages.begin(), - m_messages.end(), + Detail::g_messages.erase( + std::find_if( Detail::g_messages.begin(), + Detail::g_messages.end(), [id = message.sequence]( MessageInfo const& msg ) { return msg.sequence == id; } ) ); } void RunContext::emplaceUnscopedMessage( MessageBuilder&& builder ) { - m_messageScopes.emplace_back( CATCH_MOVE(builder) ); + Detail::g_messageScopes.emplace_back( CATCH_MOVE(builder) ); } std::string RunContext::getCurrentTestName() const { @@ -651,10 +661,10 @@ namespace Catch { m_testCaseTracker->close(); handleUnfinishedSections(); - m_messageScopes.clear(); + Detail::g_messageScopes.clear(); // TBD: At this point, m_messages should be empty. Do we want to // assert that this is true, or keep the defensive clear call? - m_messages.clear(); + Detail::g_messages.clear(); SectionStats testCaseSectionStats(CATCH_MOVE(testCaseSection), assertions, duration, missingAssertions); m_reporter->sectionEnded(testCaseSectionStats); diff --git a/src/catch2/internal/catch_run_context.hpp b/src/catch2/internal/catch_run_context.hpp index 33c0d5a1..39aca522 100644 --- a/src/catch2/internal/catch_run_context.hpp +++ b/src/catch2/internal/catch_run_context.hpp @@ -149,9 +149,6 @@ namespace Catch { Totals m_totals; Detail::AtomicCounts m_atomicAssertionCount; IEventListenerPtr m_reporter; - std::vector m_messages; - // Owners for the UNSCOPED_X information macro - std::vector m_messageScopes; std::vector m_unfinishedSections; std::vector m_activeSections; TrackerContext m_trackerContext;