WIP: thread safe assertions (still needs cleanups)

Author: horenmar

src/catch2/internal/catch_run_context.cpp

@@ -169,13 +169,18 @@ namespace Catch {
         // This is used only for CHECKED_IF/CHECKED_ELSE, and making
         // it thread-local fits that usage better than shared between threads.
         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, as it currently has to happen under mutex.
+        static thread_local bool g_clearMessageScopes = false;
+        // TODO: explain that this is used per thread for handling assertions, only makes sense per thread
+        static thread_local SourceLineInfo g_lastKnownLineInfo("DummyLocation", static_cast<size_t>(-1));;
     }
 
     RunContext::RunContext(IConfig const* _config, IEventListenerPtr&& reporter)
     :   m_runInfo(_config->name()),
         m_config(_config),
         m_reporter(CATCH_MOVE(reporter)),
-        m_lastKnownLineInfo("DummyLocation", static_cast<size_t>(-1)),
         m_outputRedirect( makeOutputRedirect( m_reporter->getPreferences().shouldRedirectStdOut ) ),
         m_abortAfterXFailedAssertions( m_config->abortAfter() ),
         m_reportAssertionStarting( m_reporter->getPreferences().shouldReportAllAssertionStarts ),
@@ -187,10 +192,12 @@ namespace Catch {
     }
 
     RunContext::~RunContext() {
+        m_totals.assertions = m_atomicAssertionCount.toCounts();
         m_reporter->testRunEnded(TestRunStats(m_runInfo, m_totals, aborting()));
     }
 
     Totals RunContext::runTest(TestCaseHandle const& testCase) {
+        m_totals.assertions = m_atomicAssertionCount.toCounts();
         const Totals prevTotals = m_totals;
 
         auto const& testInfo = testCase.getTestCaseInfo();
@@ -245,6 +252,7 @@ namespace Catch {
 
             m_reporter->testCasePartialStarting(testInfo, testRuns);
 
+            m_totals.assertions = m_atomicAssertionCount.toCounts();
             const auto beforeRunTotals = m_totals;
             runCurrentTest();
             std::string oneRunCout = m_outputRedirect->getStdout();
@@ -253,6 +261,7 @@ namespace Catch {
             redirectedCout += oneRunCout;
             redirectedCerr += oneRunCerr;
 
+            m_totals.assertions = m_atomicAssertionCount.toCounts();
             const auto singleRunTotals = m_totals.delta(beforeRunTotals);
             auto statsForOneRun = TestCaseStats(testInfo, singleRunTotals, CATCH_MOVE(oneRunCout), CATCH_MOVE(oneRunCerr), aborting());
             m_reporter->testCasePartialEnded(statsForOneRun, testRuns);
@@ -282,35 +291,35 @@ namespace Catch {
 
 
     void RunContext::assertionEnded(AssertionResult&& result) {
-        // These could be atomicized (AtomicTotals for assertions?)
-        // FIXME: grab the lineInfo here for paths that end calling into assertionEnded...
-        m_lastKnownLineInfo = result.m_info.lineInfo;
+        Detail::g_lastKnownLineInfo = result.m_info.lineInfo;
         if (result.getResultType() == ResultWas::Ok) {
-            m_totals.assertions.passed++;
+            m_atomicAssertionCount.passed++;
             Detail::g_lastAssertionPassed = true;
         } else if (result.getResultType() == ResultWas::ExplicitSkip) {
-            m_totals.assertions.skipped++;
+            m_atomicAssertionCount.skipped++;
             Detail::g_lastAssertionPassed = true;
         } else if (!result.succeeded()) {
             Detail::g_lastAssertionPassed = false;
             if (result.isOk()) {
             }
             else if( m_activeTestCase->getTestCaseInfo().okToFail() ) // Read from a shared state established before the threads could start, this is fine
-                m_totals.assertions.failedButOk++;
+                m_atomicAssertionCount.failedButOk++;
             else
-                m_totals.assertions.failed++;
+                m_atomicAssertionCount.failed++;
         }
         else {
             Detail::g_lastAssertionPassed = true;
         }
 
         // From here, we are touching shared state and need mutex.
+        std::lock_guard<std::mutex> lock( m_assertionMutex );
         {
-            if ( m_clearMessageScopes ) {
+            if ( Detail::g_clearMessageScopes ) {
                 m_messageScopes.clear();
-                m_clearMessageScopes = false;
+                Detail::g_clearMessageScopes = false;
             }
             auto _ = scopedDeactivate( *m_outputRedirect );
+            m_totals.assertions = m_atomicAssertionCount.toCounts();
             m_reporter->assertionEnded( AssertionStats( result, m_messages, m_totals ) );
         }
 
@@ -325,7 +334,7 @@ namespace Catch {
 
     void RunContext::notifyAssertionStarted( AssertionInfo const& info ) {
         if (m_reportAssertionStarting) {
-            // TODO: Mutex here
+            std::lock_guard<std::mutex> lock( m_assertionMutex );
             auto _ = scopedDeactivate( *m_outputRedirect );
             m_reporter->assertionStarting( info );
         }
@@ -344,13 +353,16 @@ namespace Catch {
         m_activeSections.push_back(&sectionTracker);
 
         SectionInfo sectionInfo( sectionLineInfo, static_cast<std::string>(sectionName) );
-        m_lastKnownLineInfo = sectionLineInfo;
+        Detail::g_lastKnownLineInfo = sectionLineInfo;
 
         {
             auto _ = scopedDeactivate( *m_outputRedirect );
             m_reporter->sectionStarting( sectionInfo );
         }
 
+        // FIXME: Do we need to change this, or do we rely on the update in assertion ended?
+        //        We probably need to update the assertion totals here, due to the fast path not updating
+        m_totals.assertions = m_atomicAssertionCount.toCounts();
         assertions = m_totals.assertions;
 
         return true;
@@ -363,7 +375,7 @@ namespace Catch {
             m_trackerContext,
             TestCaseTracking::NameAndLocationRef(
                  generatorName, lineInfo ) );
-        m_lastKnownLineInfo = lineInfo;
+        Detail::g_lastKnownLineInfo = lineInfo;
         return tracker;
     }
 
@@ -395,12 +407,13 @@ namespace Catch {
             return false;
         if (m_trackerContext.currentTracker().hasChildren())
             return false;
-        m_totals.assertions.failed++;
+        m_atomicAssertionCount.failed++;
         assertions.failed++;
         return true;
     }
 
     void RunContext::sectionEnded(SectionEndInfo&& endInfo) {
+        m_totals.assertions = m_atomicAssertionCount.toCounts();
         Counts assertions = m_totals.assertions - endInfo.prevAssertions;
         bool missingAssertions = testForMissingAssertions(assertions);
 
@@ -476,6 +489,10 @@ namespace Catch {
     }
 
     const AssertionResult * RunContext::getLastResult() const {
+        // m_lastResult is updated inside the assertion slow-path, so it needs to be mutexed as well
+        // TODO: m_lastResult is not updated in the fast path, is there a point in support it at all?
+        // TODO: also shouldn't it be a thread-local, since assertions are thread-owned until reporter?
+        std::lock_guard<std::mutex> _( m_assertionMutex );
         return &(*m_lastResult);
     }
 
@@ -487,13 +504,20 @@ namespace Catch {
         // TODO: What do we do here about threads? Give up? :-D Or maybe slap a mutex and hope for the best?
 
 
-        // TODO: scoped deactivate here? Just give up and do best effort?
-        //       the deactivation can break things further, OTOH so can the
-        //       capture
-        auto _ = scopedDeactivate( *m_outputRedirect );
+        {
+            // Lock before touching the outputs, but we can unlock while preparing
+            // the fake assertion and stuff.
+            // TODO: Should we unlock? Trying to let other threads run while fatal error
+            //       is happening is... ill-advised.
+            std::lock_guard<std::mutex> lock( m_assertionMutex );
+            // TODO: scoped deactivate here? Just give up and do best effort?
+            //       the deactivation can break things further, OTOH so can the
+            //       capture
+            auto _ = scopedDeactivate( *m_outputRedirect );
 
-        // First notify reporter that bad things happened
-        m_reporter->fatalErrorEncountered( message );
+            // First notify reporter that bad things happened
+            m_reporter->fatalErrorEncountered( message );
+        }
 
         // Don't rebuild the result -- the stringification itself can cause more fatal errors
         // Instead, fake a result data.
@@ -504,6 +528,9 @@ namespace Catch {
 
         assertionEnded(CATCH_MOVE(result) );
 
+
+        // TODO: Should we keep granular locks here, or take a single lock at the start?
+        std::lock_guard<std::mutex> lock( m_assertionMutex );
         // Best effort cleanup for sections that have not been destructed yet
         // Since this is a fatal error, we have not had and won't have the opportunity to destruct them properly
         while (!m_activeSections.empty()) {
@@ -533,6 +560,7 @@ namespace Catch {
                                   std::string(),
                                   false));
         m_totals.testCases.failed++;
+        m_totals.assertions = m_atomicAssertionCount.toCounts();
         m_reporter->testRunEnded(TestRunStats(m_runInfo, m_totals, false));
     }
 
@@ -542,24 +570,25 @@ namespace Catch {
 
     void RunContext::assertionPassedFastPath(SourceLineInfo lineInfo) {
         // We want to save the line info for better experience with unexpected assertions
-        m_lastKnownLineInfo = lineInfo;
-        ++m_totals.assertions.passed;
+        Detail::g_lastKnownLineInfo = lineInfo;
+        ++m_atomicAssertionCount.passed;
         Detail::g_lastAssertionPassed = true;
-        m_clearMessageScopes = true;
+        Detail::g_clearMessageScopes = true;
     }
 
     bool RunContext::aborting() const {
-        return m_totals.assertions.failed >= m_abortAfterXFailedAssertions;
+        return m_atomicAssertionCount.failed >= m_abortAfterXFailedAssertions;
     }
 
     void RunContext::runCurrentTest() {
         auto const& testCaseInfo = m_activeTestCase->getTestCaseInfo();
         SectionInfo testCaseSection(testCaseInfo.lineInfo, testCaseInfo.name);
         m_reporter->sectionStarting(testCaseSection);
+        m_totals.assertions = m_atomicAssertionCount.toCounts();
         Counts prevAssertions = m_totals.assertions;
         double duration = 0;
         m_shouldReportUnexpected = true;
-        m_lastKnownLineInfo = testCaseInfo.lineInfo;
+        Detail::g_lastKnownLineInfo = testCaseInfo.lineInfo;
 
         Timer timer;
         CATCH_TRY {
@@ -583,6 +612,7 @@ namespace Catch {
                                                    dummyReaction );
             }
         }
+        m_totals.assertions = m_atomicAssertionCount.toCounts();
         Counts assertions = m_totals.assertions - prevAssertions;
         bool missingAssertions = testForMissingAssertions(assertions);
 
@@ -651,7 +681,7 @@ namespace Catch {
             ITransientExpression const *expr,
             bool negated ) {
 
-        m_lastKnownLineInfo = info.lineInfo; // line info will be atomic -> potentially non-locking on non-msvc platforms, based on target arch (MSVC just sucks :shrug:)
+        Detail::g_lastKnownLineInfo = info.lineInfo;
         AssertionResultData data( resultType, LazyExpression( negated ) );
 
         AssertionResult assertionResult{ info, CATCH_MOVE( data ) };
@@ -666,7 +696,7 @@ namespace Catch {
             std::string&& message,
             AssertionReaction& reaction
     ) {
-        m_lastKnownLineInfo = info.lineInfo;
+        Detail::g_lastKnownLineInfo = info.lineInfo;
 
         AssertionResultData data( resultType, LazyExpression( false ) );
         data.message = CATCH_MOVE( message );
@@ -697,7 +727,7 @@ namespace Catch {
             std::string&& message,
             AssertionReaction& reaction
     ) {
-        m_lastKnownLineInfo = info.lineInfo;
+        Detail::g_lastKnownLineInfo = info.lineInfo;
 
         AssertionResultData data( ResultWas::ThrewException, LazyExpression( false ) );
         data.message = CATCH_MOVE(message);
@@ -715,11 +745,12 @@ namespace Catch {
 
     AssertionInfo RunContext::makeDummyAssertionInfo() {
         const bool testCaseJustStarted =
-            m_lastKnownLineInfo == m_activeTestCase->getTestCaseInfo().lineInfo;
+            Detail::g_lastKnownLineInfo ==
+            m_activeTestCase->getTestCaseInfo().lineInfo;
 
         return AssertionInfo{
             testCaseJustStarted ? "TEST_CASE"_sr : StringRef(),
-            m_lastKnownLineInfo,
+            Detail::g_lastKnownLineInfo,
             testCaseJustStarted ? StringRef() : "{Unknown expression after the reported line}"_sr,
             ResultDisposition::Normal
         };
@@ -729,7 +760,7 @@ namespace Catch {
             AssertionInfo const& info
     ) {
         using namespace std::string_literals;
-        m_lastKnownLineInfo = info.lineInfo;
+        Detail::g_lastKnownLineInfo = info.lineInfo;
 
         AssertionResultData data( ResultWas::ThrewException, LazyExpression( false ) );
         data.message = "Exception translation was disabled by CATCH_CONFIG_FAST_COMPILE"s;

src/catch2/internal/catch_run_context.hpp

@@ -21,6 +21,8 @@
 #include <catch2/internal/catch_optional.hpp>
 #include <catch2/internal/catch_move_and_forward.hpp>
 
+#include <atomic>
+#include <mutex>
 #include <string>
 
 namespace Catch {
@@ -33,6 +35,19 @@ namespace Catch {
 
     ///////////////////////////////////////////////////////////////////////////
 
+    namespace Detail {
+        struct AtomicCounts {
+            std::atomic<std::uint64_t> passed = 0;
+            std::atomic<std::uint64_t> failed = 0;
+            std::atomic<std::uint64_t> failedButOk = 0;
+            std::atomic<std::uint64_t> skipped = 0;
+
+            Counts toCounts() const {
+                return Counts{ passed, failed, failedButOk, skipped };
+            }
+        };
+    }
+
     class RunContext final : public IResultCapture {
 
     public:
@@ -113,8 +128,6 @@ namespace Catch {
         bool aborting() const;
 
     private:
-        // Fast path for handling succesful assertions if the reporters
-        // don't need to be notified.
         void assertionPassedFastPath( SourceLineInfo lineInfo );
 
         void runCurrentTest();
@@ -139,29 +152,27 @@ namespace Catch {
     private:
 
         void handleUnfinishedSections();
-
+        // TODO: Make this a recursive mutex if we use a single big mutex in the fatal error handler
+        mutable std::mutex m_assertionMutex;
         TestRunInfo m_runInfo;
         TestCaseHandle const* m_activeTestCase = nullptr;
         ITracker* m_testCaseTracker = nullptr;
         Optional<AssertionResult> m_lastResult;
 
         IConfig const* m_config;
         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;
-        SourceLineInfo m_lastKnownLineInfo;
         std::vector<SectionEndInfo> m_unfinishedSections;
         std::vector<ITracker*> m_activeSections;
         TrackerContext m_trackerContext;
         Detail::unique_ptr<OutputRedirect> m_outputRedirect;
         FatalConditionHandler m_fatalConditionhandler;
         // Caches m_config->abortAfter() to avoid vptr calls/allow inlining
         size_t m_abortAfterXFailedAssertions;
-        // Should we clear message scopes before sending off the messages to reporter?
-        // Set in `assertionPassedFastPath` to avoid doing the full clear there.
-        bool m_clearMessageScopes = false;
         bool m_shouldReportUnexpected = true;
         // Caches whether `assertionStarting` events should be sent to the reporter.
         bool m_reportAssertionStarting;

tests/SelfTest/UsageTests/Misc.tests.cpp

@@ -562,3 +562,20 @@ TEST_CASE("Validate SEH behavior - no crash for stack unwinding", "[approvals][!
 }
 
 #endif // _MSC_VER
+
+#include <thread>
+
+TEST_CASE( "threads" ) {
+    std::vector<std::thread> threads;
+    for ( size_t t = 0; t < 16; ++t) {
+        threads.emplace_back( []() {
+            for (size_t i = 0; i < 10'000; ++i) {
+                CHECK( false );
+            }
+        } );
+    }
+
+    for (auto& t : threads) {
+        t.join();
+    }
+}