Skip to content

Commit 64bb0d5

Browse files
Nurtau Toganbaymeta-codesync[bot]
authored andcommitted
Hold lock while changing atomic running_ in TaskDispatchThread (facebook#54817)
Summary: Pull Request resolved: facebook#54817 Changelog: [Internal] Fixed a bug in TaskDispatchThread. According to https://en.cppreference.com/w/cpp/thread/condition_variable.html: > Even if the shared variable is atomic, it must be modified while owning the mutex to correctly publish the modification to the waiting thread. So we need to own a lock when changing running_ in quit(). Otherwise, there is a chance that signal will get lost, which leads to deadlock on quit(). Also I did a minor refactoring by updating running_.compare_exchange_strong() to running_.exchange(). Reviewed By: javache Differential Revision: D88640446 fbshipit-source-id: 364864d58787650a23d8f835b7874bda8d762673
1 parent 9011316 commit 64bb0d5

File tree

2 files changed

+13
-6
lines changed

2 files changed

+13
-6
lines changed

packages/react-native/ReactCxxPlatform/react/threading/TaskDispatchThread.cpp

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ void TaskDispatchThread::runAsync(
7070
if (!running_) {
7171
return;
7272
}
73-
std::lock_guard<std::mutex> guard(queueLock_);
73+
std::lock_guard<std::mutex> guard(mutex_);
7474
auto dispatchTime = std::chrono::system_clock::now() + delayMs;
7575
queue_.emplace(dispatchTime, std::move(task));
7676
loopCv_.notify_one();
@@ -91,9 +91,16 @@ void TaskDispatchThread::runSync(TaskFn&& task) noexcept {
9191
}
9292

9393
void TaskDispatchThread::quit() noexcept {
94-
bool expected = true;
95-
if (!running_.compare_exchange_strong(expected, false)) {
96-
return;
94+
{
95+
/*
96+
We should hold lock even when changing atomic variable to correctly
97+
publish the modificatino to the waiting thread
98+
from: https://en.cppreference.com/w/cpp/thread/condition_variable.html
99+
*/
100+
std::unique_lock<std::mutex> lock(mutex_);
101+
if (!running_.exchange(false)) {
102+
return;
103+
}
97104
}
98105

99106
loopCv_.notify_one();
@@ -107,7 +114,7 @@ void TaskDispatchThread::loop() noexcept {
107114
folly::setThreadName(threadName_);
108115
}
109116
while (running_) {
110-
std::unique_lock<std::mutex> lock(queueLock_);
117+
std::unique_lock<std::mutex> lock(mutex_);
111118
loopCv_.wait(lock, [&]() { return !running_ || !queue_.empty(); });
112119

113120
while (!queue_.empty()) {

packages/react-native/ReactCxxPlatform/react/threading/TaskDispatchThread.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ class TaskDispatchThread {
6161

6262
void loop() noexcept;
6363

64-
std::mutex queueLock_;
64+
std::mutex mutex_;
6565
std::condition_variable loopCv_;
6666
std::priority_queue<Task> queue_;
6767
std::atomic<bool> running_{true};

0 commit comments

Comments
 (0)