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

@@ -2,7 +2,9 @@
 # Thread safety in Catch2
 
 **Contents**<br>
-[Using assertion macros from multiple threads](#using-assertion-macros-from-multiple-threads)<br>
+[Using assertion macros from spawned threads](#using-assertion-macros-from-spawned-threads)<br>
+[Assertion-like message macros and spawned threads](#assertion-like-message-macros-and-spawned-threads)<br>
+[Message macros and spawned threads](#message-macros-and-spawned-threads)<br>
 [examples](#examples)<br>
 [`STATIC_REQUIRE` and `STATIC_CHECK`](#static_require-and-static_check)<br>
 [Fatal errors and multiple threads](#fatal-errors-and-multiple-threads)<br>
@@ -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.
+Thread safety in Catch2 is currently limited to all the assertion macros,
-Interacting with benchmark macros, message macros (e.g. `INFO` or `CAPTURE`),
+and to message or message-adjacent macros (e.g. `INFO` or `WARN`).
-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
+Interacting with benchmark macros, sections macros, generator macros, or
-the way sections define test runs is incompatible with user being able
+test case macros is not thread-safe. The way sections define paths through
-to spawn threads arbitrarily, thus that limitation is here to stay.
+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<std::jthread> 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<std::jthread> 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<std::jthread> 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<std::jthread> 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
+All of `STATIC_REQUIRE`, `STATIC_REQUIRE_FALSE`, `STATIC_CHECK`, and
-`STATIC_CHECK_FALSE` are currently thread safe. This might be surprising
+`STATIC_CHECK_FALSE` are thread safe in the delayed evaluation configuration.
-given that they are a compile-time checks, but they also rely on the
-message macros to register the result with reporter at runtime.
 
 
 ## Fatal errors and multiple threads

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 ) {

src/catch2/internal/catch_message_info.cpp

@@ -10,16 +10,17 @@
 
 namespace Catch {
 
-    MessageInfo::MessageInfo(   StringRef _macroName,
+    MessageInfo::MessageInfo( StringRef _macroName,
-                                SourceLineInfo const& _lineInfo,
+                              SourceLineInfo const& _lineInfo,
-                                ResultWas::OfType _type )
+                              ResultWas::OfType _type )
     :   macroName( _macroName ),
         lineInfo( _lineInfo ),
         type( _type ),
         sequence( ++globalCount )
     {}
 
-    // This may need protecting if threading support is added
+    // Messages are owned by their individual threads, so the counter should be thread-local as well.
-    unsigned int MessageInfo::globalCount = 0;
+    // 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

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

src/catch2/internal/catch_reusable_string_stream.cpp

@@ -7,6 +7,7 @@
 // SPDX-License-Identifier: BSL-1.0
 #include <catch2/internal/catch_reusable_string_stream.hpp>
 #include <catch2/internal/catch_singletons.hpp>
+#include <catch2/internal/catch_thread_support.hpp>
 #include <catch2/internal/catch_unique_ptr.hpp>
 
 #include <cstdio>
@@ -20,8 +21,10 @@ namespace Catch {
         std::vector<Detail::unique_ptr<std::ostringstream>> m_streams;
         std::vector<std::size_t> 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<std::ostringstream>() );
                 return m_streams.size()-1;
@@ -33,9 +36,13 @@ namespace Catch {
             }
         }
 
-        void release( std::size_t index ) {
+        void release( std::size_t index, std::ostream* originalPtr ) {
-            m_streams[index]->copyfmt( m_referenceStream ); // Restore initial flags and other state
+            assert( originalPtr );
-            m_unused.push_back(index);
+            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<std::ostringstream*>( m_oss )->str("");
         m_oss->clear();
-        Singleton<StringStreams>::getMutable().release( m_index );
+        Singleton<StringStreams>::getMutable().release( m_index, m_oss );
     }
 
     std::string ReusableStringStream::str() const {

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<size_t>(-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<MessageInfo> g_messages;
+
+        // Owners for the UNSCOPED_X information macro
+        static thread_local std::vector<ScopedMessage> 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) {
+    void RunContext::pushScopedMessage( MessageInfo const& message ) {
-        m_messages.push_back(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(
+        Detail::g_messages.erase(
-            std::find_if( m_messages.begin(),
+            std::find_if( Detail::g_messages.begin(),
-                          m_messages.end(),
+                          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);

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<MessageInfo> m_messages;
-        // Owners for the UNSCOPED_X information macro
-        std::vector<ScopedMessage> m_messageScopes;
         std::vector<SectionEndInfo> m_unfinishedSections;
         std::vector<ITracker*> m_activeSections;
         TrackerContext m_trackerContext;