Skip to content

Mark Waiter m_cv as guarded by m_mutex #295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanofsky merged 1 commit into from
Jun 11, 2026
Merged

Mark Waiter m_cv as guarded by m_mutex #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/mp/proxy-io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ struct Waiter
}

template <class Predicate>
void wait(Lock& lock, Predicate pred)
void wait(Lock& lock, Predicate pred) MP_REQUIRES(m_mutex)
{
m_cv.wait(lock.m_lock, [&]() MP_REQUIRES(m_mutex) {
// Important for this to be "while (m_fn)", not "if (m_fn)" to avoid
Expand All @@ -410,7 +410,7 @@ struct Waiter
//! EventLoop::m_mutex as long as Waiter::mutex is locked first and
//! EventLoop::m_mutex is locked second.
Mutex m_mutex;
std::condition_variable m_cv;
std::condition_variable m_cv MP_GUARDED_BY(m_mutex);
std::optional<kj::Function<void()>> m_fn MP_GUARDED_BY(m_mutex);
};

Expand Down
1 change: 1 addition & 0 deletions test/mp/test/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,7 @@ KJ_TEST("Make simultaneous IPC calls on single remote thread")
[&running, &tc, i](auto&& results) {
assert(results.getResult() == static_cast<int32_t>(100 * (i+1)));
running -= 1;
Lock lock(tc.waiter->m_mutex);

@ryanofsky ryanofsky Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

tc.waiter->m_cv.notify_all();
}));
}
Expand Down
Loading