Mark Waiter m_cv as guarded by m_mutex #295

pull maflcko wants to merge 1 commits into bitcoin-core:master from maflcko:2606-clang-tsa changing 2 files +3 −2

maflcko commented at 1:28 PM on June 10, 2026: contributor

This change fixes a lost wakeup bug in the "Make simultaneous IPC calls on single remote thread" test which is probably responsible for the hang reported https://github.com/bitcoin/bitcoin/issues/35491. The bug has existed since the test was introduced in 1238170f68e8fa7ae41c79465df5cdae34d568e9.

The change adds an MP_GUARDED_BY(m_mutex) annotation to Waiter::m_cv, and locks the mutex before notifying the condition variable in the test to satisfy the guarded-by requirement.

Locking the mutex prevents a hang in the test that could happen because if there is no lock between the running -= 1 state update and notifying the condition variable, there is no guarantee that the test is actively waiting on the condition variable when the notify call is made, so the notify might do nothing, and the test could wait forever.

(edited by ryanofsky)
Mark Waiter m_cv as guarded by m_mutex fa35501c4f
DrahtBot commented at 1:28 PM on June 10, 2026: none



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ryanofsky

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Type	Reviewers
ACK	ryanofsky

in test/mp/test/test.cpp:513 in fa35501c4f

 509 | @@ -510,6 +510,7 @@ KJ_TEST("Make simultaneous IPC calls on single remote thread")
 510 |                  [&running, &tc, i](auto&& results) {
 511 |                      assert(results.getResult() == static_cast<int32_t>(100 * (i+1)));
 512 |                      running -= 1;
 513 | +                    Lock lock(tc.waiter->m_mutex);

ryanofsky commented at 12:51 AM on June 11, 2026:

In commit "Mark Waiter m_cv as guarded by m_mutex" (fa35501c4f0ac57ffe18479791ddf209c817aa39)

Note if we wanted to make this more optimal and avoid the "hurry up and wait" scenario described in https://en.cppreference.com/cpp/thread/condition_variable/notify_one, we would unlock before notifying as recommended there, which the MP_GUARDED_BY annotation prevents:

--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -511,6 +511,7 @@ KJ_TEST("Make simultaneous IPC calls on single remote thread")
                     assert(results.getResult() == static_cast<int32_t>(100 * (i+1)));
                     running -= 1;
                     Lock lock(tc.waiter->m_mutex);
+                    lock.unlock();
                     tc.waiter->m_cv.notify_all();
                 }));
         }

Still worth having the annotation here, but this shows why the annotation could be undesirable in other cases.

ryanofsky approved
ryanofsky commented at 1:00 AM on June 11, 2026: collaborator

Code review ACK fa35501c4f0ac57ffe18479791ddf209c817aa39. Thanks for the fix!

re: #295#issue-4631671320

This allows to catch possible deadlock bugs at compile time.

For bitcoin/bitcoin#35491

I think PR description should be clearer that this does fix a specific deadlock not just "possible deadlock bugs", so I might edit it before merging (hope you don't mind)
ryanofsky merged this on Jun 11, 2026
ryanofsky closed this on Jun 11, 2026
maflcko deleted the branch on Jun 11, 2026

Contributors

maflcko

DrahtBot

ryanofsky

Mark Waiter m_cv as guarded by m_mutex #295

Reviews