Skip to content

Commit

Permalink
Fix randomly failing scheduler_tests test
Browse files Browse the repository at this point in the history
Summary
---

This closes issue Bitcoin-ABC#266. The root cause is that the original test author
was misusing threads and/or synchronization primitives. There is no
guarantee that the first main thread runs before the last scheduled
task. As such, sometimes, the last task runs and writes "42" into the
`counter` before the `BOOST_CHECK_EQUAL(counter, 0)` line gets a chance
to be evaluated in the main thread.

The fix is to not rely on undefined behavior here and instead do things
properly.  We just save the counter var to a second atomic and check
everything at the end after the two subordinate tasks have all
definitely finished running.

Test Plan
---

- `ninja all check`
- Try and reproduce the issue in Bitcoin-ABC#266 as described. If you can reproduce
  it against master but not here, then you can be happy this is fixed.

An alternative way to test if you *can't* reproduce the failure against master
is to:

1. `git checkout master`
2. Edit `src/test/scheduler_tests.cpp` and insert a
   `std::this_thread::sleep_for(std::chrono::milliseconds(21));` right
*before* the line in the `schedule_every` test that does
`BOOST_CHECK_EQUAL(counter, 0);`. This will reproduce the failure every
time.
3. `ninja test_bitcoin && src/test/test_bitcoin -t scheduler_tests`
4. `git checkout THIS_MR_BRANCH`
5. Add the sleep call in approximately the same place and do steps (2 & 3)
   again.  You should never get a failure now.
  • Loading branch information
cculianu committed Apr 22, 2021
1 parent cc5d181 commit b1d47fa
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions src/test/scheduler_tests.cpp
Expand Up @@ -138,11 +138,11 @@ BOOST_AUTO_TEST_CASE(schedule_every) {
CScheduler scheduler;

std::condition_variable cvar;
std::atomic<int> counter{15};
std::atomic<int> counter{15}, savedCounter{-1};
std::atomic<bool> keepRunning{true};

scheduler.scheduleEvery(
[&keepRunning, &cvar, &counter, &scheduler]() {
[&keepRunning, &cvar, &counter, &savedCounter, &scheduler]() {
assert(counter > 0);
cvar.notify_all();
if (--counter > 0) {
Expand All @@ -160,7 +160,7 @@ BOOST_AUTO_TEST_CASE(schedule_every) {

// We set the counter to some magic value to check the scheduler
// empty its queue properly after 120ms.
scheduler.scheduleFromNow([&counter]() { counter = 42; }, 120);
scheduler.scheduleFromNow([&counter, &savedCounter]() { savedCounter = counter.exchange(42); }, 120);
return false;
},
5);
Expand All @@ -176,10 +176,10 @@ BOOST_AUTO_TEST_CASE(schedule_every) {
BOOST_CHECK(counter >= 0);
}

BOOST_CHECK_EQUAL(counter, 0);
scheduler.stop(true);
schedulerThread.join();
BOOST_CHECK_EQUAL(counter, 42);
BOOST_CHECK_EQUAL(savedCounter, 0);
}

BOOST_AUTO_TEST_CASE(singlethreadedscheduler_ordered) {
Expand Down

0 comments on commit b1d47fa

Please sign in to comment.