From fa1bc1fe5152f9138da336d3ded9bb68c4062743 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 9 Apr 2026 13:01:51 +0200 Subject: [PATCH] test: Check btcsignals determinism in thread_safety test case The test only checked that the single atomic value is greater than 3000. However, by splitting the atomic into two, one can do one exact check, and also increase the lower bound on the inexact check. Also, test disconnect races for every second step, instead of only once at the end (likely when only one thread is running anyway). Both changes make the test stricter and may catch non-determinism issues that are not detected by sanitizers alone. The test added in this commit should also pass when applied on top of commit 63c68e2a3f98d2466a7e766d861ba3a94e92cd20, which is still using the boost implementation. --- src/test/btcsignals_tests.cpp | 36 +++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/test/btcsignals_tests.cpp b/src/test/btcsignals_tests.cpp index 7d7970bdb59..dd7fcb9b3bd 100644 --- a/src/test/btcsignals_tests.cpp +++ b/src/test/btcsignals_tests.cpp @@ -111,17 +111,18 @@ BOOST_AUTO_TEST_CASE(any_of_combiner) * Connect sig0 to an incrementor function and loop in a thread. * Meanwhile, in another thread, inject and call new increment callbacks. * Both threads are constantly calling empty/connected. - * Though the end-result is undefined due to a non-deterministic number of - * total callbacks executed, this should all be completely threadsafe. + * The end-result must be deterministic for the atomic modified by conn0. + * Though, the end-result for the atomic modified by the extra connections is + * undefined due to a non-deterministic number of total callbacks executed. + * In any case, this should all be completely threadsafe. * Sanitizers should pick up any buggy data race behavior (if present). */ BOOST_AUTO_TEST_CASE(thread_safety) { btcsignals::signal sig0; - std::atomic val{0}; - auto conn0 = sig0.connect([&val] { - val++; - }); + std::atomic val_det{0}; + std::atomic val_non_det{0}; + auto conn0 = sig0.connect([&val_det] { val_det++; }); std::thread incrementor([&conn0, &sig0] { for (int i = 0; i < 1000; i++) { @@ -134,16 +135,17 @@ BOOST_AUTO_TEST_CASE(thread_safety) assert(conn0.connected()); }); - std::thread extra_increment_injector([&conn0, &sig0, &val] { + std::thread extra_increment_injector([&conn0, &sig0, &val_non_det] { static constexpr size_t num_extra_conns{1000}; std::vector extra_conns; extra_conns.reserve(num_extra_conns); for (size_t i = 0; i < num_extra_conns; i++) { BOOST_CHECK(!sig0.empty()); BOOST_CHECK(conn0.connected()); - extra_conns.emplace_back(sig0.connect([&val] { - val++; - })); + btcsignals::scoped_connection extra{sig0.connect([&val_non_det] { val_non_det++; })}; + if (i % 2 == 0) { + extra_conns.emplace_back(std::move(extra)); + } sig0(); } }); @@ -152,10 +154,16 @@ BOOST_AUTO_TEST_CASE(thread_safety) conn0.disconnect(); BOOST_CHECK(sig0.empty()); - // sig will have been called 2000 times, and at least 1000 of those will - // have been executing multiple incrementing callbacks. So while val is - // probably MUCH bigger, it's guaranteed to be at least 3000. - BOOST_CHECK_GE(val.load(), 3000); + // sig0 will have been called 2000 times, and only the first connection did + // increment val_det, so it must be 2000. + BOOST_CHECK_EQUAL(val_det.load(), 2000); + // The number of connections that increment val_non_det is growing from 1 + // to 500, where 500 are disconnected immediately again after the step. + // Before the end of each step the connections are called at least once. + // However, it is unknown how often the connections have been called + // exactly. The 500th Triangular Number gives a lower estimate. + // T_n=n(n+1)/2 + BOOST_CHECK_GE(val_non_det.load(), 500 * 501 / 2); } /* Test that connection and disconnection works from within signal