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.

6 files changed, 52 insertions, 36 deletions

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