From 4e6fe3450137909f0bcf969815fb1a84d2d8d2dc Mon Sep 17 00:00:00 2001 From: "Raphael S. Carvalho" Date: Fri, 6 Oct 2023 15:04:59 -0300 Subject: [PATCH] tests: Synchronize boost logger for multithreaded tests in sstable_directory_test The logger is not thread safe, so a multithreaded test can concurrently write into the log, yielding unreadable XMLs. Example: boost/sstable_directory_test: failed to parse XML output '/scylladir/testlog/x86_64/release/xml/boost.sstable_directory_test.sstable_directory_shared_sstables_reshard_correctly.3.xunit.xml': not well-formed (invalid token): line 1, column 1351 The critical (today's unprotected) section is in boost/test/utils/xml_printer.hpp: ``` inline std::ostream& operator<<( custom_printer const& p, const_string value ) { *p << BOOST_TEST_L( "" ); } ``` The problem is not restricted to xml, but the unreadable xml file caused the test to fail when trying to parse it, to present a summary. New thread-safe variants of BOOST_REQUIRE and BOOST_REQUIRE_EQUAL are introduced to help multithreaded tests. We'll start patching tests of sstable_directory_test that will call BOOST_REQUIRE* from multiple threads. Later, we can expand its usage to other tests. Fixes #15654. Signed-off-by: Raphael S. Carvalho Closes scylladb/scylladb#15655 --- test/boost/sstable_directory_test.cc | 10 +++++----- test/lib/test_utils.cc | 2 ++ test/lib/test_utils.hh | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/test/boost/sstable_directory_test.cc b/test/boost/sstable_directory_test.cc index b8804997c9..e7440bb1f0 100644 --- a/test/boost/sstable_directory_test.cc +++ b/test/boost/sstable_directory_test.cc @@ -312,8 +312,8 @@ SEASTAR_THREAD_TEST_CASE(sstable_directory_test_generation_sanity) { sstdir.invoke_on_all([&] (sstables::sstable_directory& sstdir) { return seastar::async([&] { sstdir.do_for_each_sstable([&] (const shared_sstable& sst) { - BOOST_REQUIRE(sst->generation() == sst1->generation()); - BOOST_REQUIRE(!gen1_seen[this_shard_id()]); + THREADSAFE_BOOST_REQUIRE(sst->generation() == sst1->generation()); + THREADSAFE_BOOST_REQUIRE(!gen1_seen[this_shard_id()]); gen1_seen[this_shard_id()] = true; return make_ready_future<>(); }).get(); @@ -330,12 +330,12 @@ future<> verify_that_all_sstables_are_local(sharded& sstdir, return d.do_for_each_sstable([count] (sstables::shared_sstable sst) { count->fetch_add(1, std::memory_order_relaxed); auto shards = sst->get_shards_for_this_sstable(); - BOOST_REQUIRE_EQUAL(shards.size(), 1); - BOOST_REQUIRE_EQUAL(shards[0], this_shard_id()); + THREADSAFE_BOOST_REQUIRE_EQUAL(shards.size(), 1); + THREADSAFE_BOOST_REQUIRE_EQUAL(shards[0], this_shard_id()); return make_ready_future<>(); }); }).then([count = count.get(), expected_sstables] { - BOOST_REQUIRE_EQUAL(count->load(std::memory_order_relaxed), expected_sstables); + THREADSAFE_BOOST_REQUIRE_EQUAL(count->load(std::memory_order_relaxed), expected_sstables); return make_ready_future<>(); }); }); diff --git a/test/lib/test_utils.cc b/test/lib/test_utils.cc index a4e63ba001..db9c7f68ca 100644 --- a/test/lib/test_utils.cc +++ b/test/lib/test_utils.cc @@ -98,4 +98,6 @@ future<> touch_file(std::string name) { co_await f.close(); } +std::mutex boost_logger_mutex; + } diff --git a/test/lib/test_utils.hh b/test/lib/test_utils.hh index c55761b71b..5f6d56e40d 100644 --- a/test/lib/test_utils.hh +++ b/test/lib/test_utils.hh @@ -61,4 +61,18 @@ extern boost::test_tools::assertion_result has_scylla_test_env(boost::unit_test: future compare_files(std::string fa, std::string fb); future<> touch_file(std::string name); +extern std::mutex boost_logger_mutex; + +#define THREADSAFE_BOOST_CHECK( BOOST_CHECK_EXPR ) { \ + std::lock_guard guard(tests::boost_logger_mutex); \ + BOOST_CHECK_EXPR; \ + } + +// Thread-safe variants of BOOST macros for unit tests +// This is required to address lack of synchronization in boost test logger, which is susceptible to races +// in multi-threaded tests. Boost's XML printer (see boost/test/utils/xml_printer.hpp) can generate +// unreadable XML files, and therefore cause failure when parsing its content. +#define THREADSAFE_BOOST_REQUIRE( P ) THREADSAFE_BOOST_CHECK(BOOST_REQUIRE( P )) +#define THREADSAFE_BOOST_REQUIRE_EQUAL( L, R ) THREADSAFE_BOOST_CHECK(BOOST_REQUIRE_EQUAL( L, R )) + }