Make assertions thread-safe

.github/workflows/linux-other-builds.yml

@@ -69,6 +69,22 @@ jobs:
           #   cmake_configurations: -DMEMORYCHECK_COMMAND=`which valgrind` -DMEMORYCHECK_COMMAND_OPTIONS="-q --track-origins=yes --leak-check=full --num-callers=50 --show-leak-kinds=definite --error-exitcode=1"
           #   other_ctest_args: -T memcheck -LE uses-python
 
+          # Thread sanitizer test gcc-7
+          - cxx: g++-7
+            build_description: Thread sanitizer tests
+            build_type: Debug
+            std: 14
+            other_pkgs: g++-7
+            cmake_configurations: -DCATCH_BUILD_EXTRA_TESTS=ON -DCMAKE_CXX_FLAGS="-fsanitize=thread"
+
+          # Thread sanitizer test clang-10
+          - cxx: clang++-10
+            build_description: Thread sanitizer tests
+            build_type: Debug
+            std: 14
+            other_pkgs: clang-10
+            cmake_configurations: -DCATCH_BUILD_EXTRA_TESTS=ON -DCMAKE_CXX_FLAGS="-fsanitize=thread"
+
 
     steps:
     - uses: actions/checkout@v4

CMakeLists.txt

@@ -20,6 +20,7 @@ cmake_dependent_option(CATCH_BUILD_TESTING "Build the SelfTest project" ON "CATC
 cmake_dependent_option(CATCH_BUILD_EXAMPLES "Build code examples" OFF "CATCH_DEVELOPMENT_BUILD" OFF)
 cmake_dependent_option(CATCH_BUILD_EXTRA_TESTS "Build extra tests" OFF "CATCH_DEVELOPMENT_BUILD" OFF)
 cmake_dependent_option(CATCH_BUILD_FUZZERS "Build fuzzers" OFF "CATCH_DEVELOPMENT_BUILD" OFF)
+cmake_dependent_option(CATCH_BUILD_BENCHMARKS "Build benchmarks" OFF "CATCH_DEVELOPMENT_BUILD" OFF)
 cmake_dependent_option(CATCH_ENABLE_COVERAGE "Generate coverage for codecov.io" OFF "CATCH_DEVELOPMENT_BUILD" OFF)
 cmake_dependent_option(CATCH_ENABLE_WERROR "Enables Werror during build" ON "CATCH_DEVELOPMENT_BUILD" OFF)
 cmake_dependent_option(CATCH_BUILD_SURROGATES "Enable generating and building surrogate TUs for the main headers" OFF "CATCH_DEVELOPMENT_BUILD" OFF)
@@ -102,6 +103,11 @@ if(CATCH_BUILD_FUZZERS)
   add_subdirectory(fuzzing)
 endif()
 
+if(CATCH_BUILD_BENCHMARKS)
+    set(CMAKE_FOLDER "benchmarks")
+    add_subdirectory(benchmarks)
+endif()
+
 if(CATCH_DEVELOPMENT_BUILD)
   set(CATCH_ALL_TARGETS ${CATCH_IMPL_TARGETS} ${CATCH_TEST_TARGETS})
   add_warnings_to_targets("${CATCH_ALL_TARGETS}")

benchmarks/CMakeLists.txt

@@ -0,0 +1,6 @@
+add_executable(Benchmarks catch_benchmarks.cpp)
+target_link_libraries(Benchmarks PRIVATE Catch2WithMain)
+target_compile_features(Benchmarks PUBLIC cxx_std_14)
+
+list(APPEND CATCH_TEST_TARGETS Benchmarks)
+set(CATCH_TEST_TARGETS ${CATCH_TEST_TARGETS} PARENT_SCOPE)

benchmarks/catch_benchmarks.cpp

@@ -0,0 +1,25 @@
+#include <catch2/catch_test_macros.hpp>
+#include <catch2/benchmark/catch_benchmark.hpp>
+
+#include <mutex>
+
+std::recursive_mutex global_lock;
+
+int no_lock() {
+    return 2;
+}
+
+int take_lock() {
+    std::unique_lock<std::recursive_mutex> lock(global_lock);
+    return 2;
+}
+
+TEST_CASE("std::recursive_mutex overhead benchmark", "[benchmark][mutex]") {
+    BENCHMARK("no lock") {
+        return no_lock();
+    };
+
+    BENCHMARK("with std::recursive_mutex") {
+        return take_lock();
+    };
+}

docs/limitations.md

@@ -58,23 +58,9 @@ again.
 This section outlines some missing features, what is their status and their possible workarounds.
 
 ### Thread safe assertions
-Catch2's assertion macros are not thread safe. This does not mean that
-you cannot use threads inside Catch's test, but that only single thread
-can interact with Catch's assertions and other macros.
+Catch2's assertion macros and logging macros are thread safe.
 
-This means that this is ok
-```cpp
-    std::vector<std::thread> threads;
-    std::atomic<int> cnt{ 0 };
-    for (int i = 0; i < 4; ++i) {
-        threads.emplace_back([&]() {
-            ++cnt; ++cnt; ++cnt; ++cnt;
-        });
-    }
-    for (auto& t : threads) { t.join(); }
-    REQUIRE(cnt == 16);
-```
-because only one thread passes the `REQUIRE` macro and this is not
+This is ok however it was previously not ok for Catch2 3.8.0 and earlier:
 ```cpp
     std::vector<std::thread> threads;
     std::atomic<int> cnt{ 0 };
@@ -88,8 +74,6 @@ because only one thread passes the `REQUIRE` macro and this is not
     REQUIRE(cnt == 16);
 ```
 
-We currently do not plan to support thread-safe assertions.
-
 
 ### Process isolation in a test
 Catch does not support running tests in isolated (forked) processes. While this might in the future, the fact that Windows does not support forking and only allows full-on process creation and the desire to keep code as similar as possible across platforms, mean that this is likely to take significant development time, that is not currently available.

src/CMakeLists.txt

@@ -182,6 +182,7 @@ set(IMPL_SOURCES
   ${SOURCES_DIR}/internal/catch_fatal_condition_handler.cpp
   ${SOURCES_DIR}/internal/catch_floating_point_helpers.cpp
   ${SOURCES_DIR}/internal/catch_getenv.cpp
+  ${SOURCES_DIR}/internal/catch_global_lock.cpp
   ${SOURCES_DIR}/internal/catch_istream.cpp
   ${SOURCES_DIR}/internal/catch_jsonwriter.cpp
   ${SOURCES_DIR}/internal/catch_lazy_expr.cpp

src/catch2/catch_all.hpp

@@ -73,6 +73,7 @@
 #include <catch2/internal/catch_fatal_condition_handler.hpp>
 #include <catch2/internal/catch_floating_point_helpers.hpp>
 #include <catch2/internal/catch_getenv.hpp>
+#include <catch2/internal/catch_global_lock.hpp>
 #include <catch2/internal/catch_is_permutation.hpp>
 #include <catch2/internal/catch_istream.hpp>
 #include <catch2/internal/catch_jsonwriter.hpp>

src/catch2/internal/catch_assertion_handler.cpp

@@ -10,9 +10,12 @@
 #include <catch2/internal/catch_context.hpp>
 #include <catch2/internal/catch_debugger.hpp>
 #include <catch2/internal/catch_test_failure_exception.hpp>
+#include <catch2/internal/catch_global_lock.hpp>
 #include <catch2/matchers/catch_matchers_string.hpp>
 
 namespace Catch {
+    // The AssertionHandler API and handleExceptionMatchExpr are used by assertion macros. Everything here must be
+    // locked as catch internals are not thread-safe.
 
     AssertionHandler::AssertionHandler
         (   StringRef macroName,
@@ -22,13 +25,23 @@ namespace Catch {
     :   m_assertionInfo{ macroName, lineInfo, capturedExpression, resultDisposition },
         m_resultCapture( getResultCapture() )
     {
+        auto lock = take_global_lock();
         m_resultCapture.notifyAssertionStarted( m_assertionInfo );
     }
 
+    AssertionHandler::~AssertionHandler() {
+        auto lock = take_global_lock();
+        if ( !m_completed ) {
+            m_resultCapture.handleIncomplete( m_assertionInfo );
+        }
+    }
+
     void AssertionHandler::handleExpr( ITransientExpression const& expr ) {
+        auto lock = take_global_lock();
         m_resultCapture.handleExpr( m_assertionInfo, expr, m_reaction );
     }
     void AssertionHandler::handleMessage(ResultWas::OfType resultType, std::string&& message) {
+        auto lock = take_global_lock();
         m_resultCapture.handleMessage( m_assertionInfo, resultType, CATCH_MOVE(message), m_reaction );
     }
 
@@ -55,21 +68,26 @@ namespace Catch {
     }
 
     void AssertionHandler::handleUnexpectedInflightException() {
+        auto lock = take_global_lock();
         m_resultCapture.handleUnexpectedInflightException( m_assertionInfo, Catch::translateActiveException(), m_reaction );
     }
 
     void AssertionHandler::handleExceptionThrownAsExpected() {
+        auto lock = take_global_lock();
         m_resultCapture.handleNonExpr(m_assertionInfo, ResultWas::Ok, m_reaction);
     }
     void AssertionHandler::handleExceptionNotThrownAsExpected() {
+        auto lock = take_global_lock();
         m_resultCapture.handleNonExpr(m_assertionInfo, ResultWas::Ok, m_reaction);
     }
 
     void AssertionHandler::handleUnexpectedExceptionNotThrown() {
+        auto lock = take_global_lock();
         m_resultCapture.handleUnexpectedExceptionNotThrown( m_assertionInfo, m_reaction );
     }
 
     void AssertionHandler::handleThrowingCallSkipped() {
+        auto lock = take_global_lock();
         m_resultCapture.handleNonExpr(m_assertionInfo, ResultWas::Ok, m_reaction);
     }
 

src/catch2/internal/catch_assertion_handler.hpp

@@ -34,12 +34,7 @@ namespace Catch {
                 SourceLineInfo const& lineInfo,
                 StringRef capturedExpression,
                 ResultDisposition::Flags resultDisposition );
-        ~AssertionHandler() {
-            if ( !m_completed ) {
-                m_resultCapture.handleIncomplete( m_assertionInfo );
-            }
-        }
-
+        ~AssertionHandler();
 
         template<typename T>
         constexpr void handleExpr( ExprLhs<T> const& expr ) {

src/catch2/internal/catch_global_lock.cpp

@@ -0,0 +1,23 @@
+
+//              Copyright Catch2 Authors
+// Distributed under the Boost Software License, Version 1.0.
+//   (See accompanying file LICENSE.txt or copy at
+//        https://www.boost.org/LICENSE_1_0.txt)
+
+// SPDX-License-Identifier: BSL-1.0
+#include <catch2/internal/catch_global_lock.hpp>
+#include <catch2/internal/catch_compiler_capabilities.hpp>
+
+CATCH_INTERNAL_START_WARNINGS_SUPPRESSION
+CATCH_INTERNAL_SUPPRESS_GLOBALS_WARNINGS
+
+namespace Catch {
+
+    std::recursive_mutex& get_global_lock() {
+        static std::recursive_mutex global_lock;
+        return global_lock;
+    }
+
+} // namespace Catch
+
+CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION

src/catch2/internal/catch_global_lock.hpp

@@ -0,0 +1,24 @@
+
+//              Copyright Catch2 Authors
+// Distributed under the Boost Software License, Version 1.0.
+//   (See accompanying file LICENSE.txt or copy at
+//        https://www.boost.org/LICENSE_1_0.txt)
+
+// SPDX-License-Identifier: BSL-1.0
+
+#ifndef CATCH_GLOBAL_LOCK_HPP_INCLUDED
+#define CATCH_GLOBAL_LOCK_HPP_INCLUDED
+
+#include <mutex>
+
+namespace Catch {
+
+    std::recursive_mutex& get_global_lock();
+
+    inline auto take_global_lock() {
+        return std::unique_lock<std::recursive_mutex>(get_global_lock());
+    }
+
+} // namespace Catch
+
+#endif // CATCH_GLOBAL_LOCK_HPP_INCLUDED

src/catch2/internal/catch_reusable_string_stream.cpp

@@ -8,6 +8,7 @@
 #include <catch2/internal/catch_reusable_string_stream.hpp>
 #include <catch2/internal/catch_singletons.hpp>
 #include <catch2/internal/catch_unique_ptr.hpp>
+#include <catch2/internal/catch_global_lock.hpp>
 
 #include <cstdio>
 #include <sstream>
@@ -39,12 +40,18 @@ namespace Catch {
         }
     };
 
-    ReusableStringStream::ReusableStringStream()
-    :   m_index( Singleton<StringStreams>::getMutable().add() ),
-        m_oss( Singleton<StringStreams>::getMutable().m_streams[m_index].get() )
-    {}
+    // Catch message macros create MessageStreams which hold ReusableStringStream. Since catch internals are not
+    // thread-safe locking is needed and it's easiest to lock at the ReusableStringStream construct/destruct level
+    // instead of poking around StringStreams and Singleton.
+
+    ReusableStringStream::ReusableStringStream() {
+        auto lock = take_global_lock();
+        m_index = Singleton<StringStreams>::getMutable().add();
+        m_oss = Singleton<StringStreams>::getMutable().m_streams[m_index].get();
+    }
 
     ReusableStringStream::~ReusableStringStream() {
+        auto lock = take_global_lock();
         static_cast<std::ostringstream*>( m_oss )->str("");
         m_oss->clear();
         Singleton<StringStreams>::getMutable().release( m_index );

src/catch2/internal/catch_run_context.cpp

@@ -21,6 +21,7 @@
 #include <catch2/internal/catch_assertion_handler.hpp>
 #include <catch2/internal/catch_test_failure_exception.hpp>
 #include <catch2/internal/catch_result_type.hpp>
+#include <catch2/internal/catch_global_lock.hpp>
 
 #include <cassert>
 #include <algorithm>
@@ -418,19 +419,25 @@ namespace Catch {
         m_unfinishedSections.push_back(CATCH_MOVE(endInfo));
     }
 
+    // Catch benchmark macros call these functions. Since catch internals are not thread-safe locking is needed.
+
     void RunContext::benchmarkPreparing( StringRef name ) {
+        auto lock = take_global_lock();
         auto _ = scopedDeactivate( *m_outputRedirect );
         m_reporter->benchmarkPreparing( name );
     }
     void RunContext::benchmarkStarting( BenchmarkInfo const& info ) {
+        auto lock = take_global_lock();
         auto _ = scopedDeactivate( *m_outputRedirect );
         m_reporter->benchmarkStarting( info );
     }
     void RunContext::benchmarkEnded( BenchmarkStats<> const& stats ) {
+        auto lock = take_global_lock();
         auto _ = scopedDeactivate( *m_outputRedirect );
         m_reporter->benchmarkEnded( stats );
     }
     void RunContext::benchmarkFailed( StringRef error ) {
+        auto lock = take_global_lock();
         auto _ = scopedDeactivate( *m_outputRedirect );
         m_reporter->benchmarkFailed( error );
     }

src/catch2/meson.build

@@ -223,6 +223,7 @@ internal_sources = files(
   'internal/catch_fatal_condition_handler.cpp',
   'internal/catch_floating_point_helpers.cpp',
   'internal/catch_getenv.cpp',
+  'internal/catch_global_lock.cpp',
   'internal/catch_istream.cpp',
   'internal/catch_jsonwriter.cpp',
   'internal/catch_lazy_expr.cpp',

tests/ExtraTests/CMakeLists.txt

@@ -429,6 +429,21 @@ set_tests_properties(Reporters::CrashInJunitReporter
     LABELS "uses-signals"
 )
 
+add_executable(Multithreading ${TESTS_DIR}/X37-Multithreading.cpp)
+find_package(Threads REQUIRED)
+target_link_libraries(Multithreading PRIVATE Catch2::Catch2WithMain Threads::Threads)
+target_compile_features(Multithreading PRIVATE cxx_std_11)
+add_test(
+  NAME Multithreading::Multithreading
+  COMMAND ${CMAKE_COMMAND} -E env $<TARGET_FILE:Multithreading>
+)
+set_tests_properties(
+  Multithreading::Multithreading
+  PROPERTIES
+    PASS_REGULAR_EXPRESSION "passed"
+    FAIL_REGULAR_EXPRESSION "ThreadSanitizer"
+)
+
 add_executable(AssertionStartingEventGoesBeforeAssertionIsEvaluated
   X20-AssertionStartingEventGoesBeforeAssertionIsEvaluated.cpp
 )

tests/ExtraTests/X37-Multithreading.cpp

@@ -0,0 +1,37 @@
+
+//              Copyright Catch2 Authors
+// Distributed under the Boost Software License, Version 1.0.
+//   (See accompanying file LICENSE.txt or copy at
+//        https://www.boost.org/LICENSE_1_0.txt)
+
+// SPDX-License-Identifier: BSL-1.0
+
+#include <catch2/catch_test_macros.hpp>
+#include <catch2/internal/catch_textflow.hpp>
+#include <catch2/benchmark/catch_benchmark.hpp>
+
+#include <thread>
+#include <atomic>
+
+TEST_CASE( "ThreadAssertionTest",
+           "[Multithreading]" ) {
+    SECTION( "Basic" ) {
+        std::atomic_bool should_stop{false};
+        std::thread a([&should_stop] () {
+            while (!should_stop) {
+                FAIL_CHECK(false);
+                CHECK(true);
+            }
+        });
+        std::thread b([&should_stop] () {
+            while (!should_stop) {
+                FAIL_CHECK(false);
+                CHECK(true);
+            }
+        });
+        std::this_thread::sleep_for( std::chrono::milliseconds( 1'000 ) );
+        should_stop = true;
+        a.join();
+        b.join();
+    }
+}