Skip to content

Commit ad5949f

Browse files
Nurtau Toganbayfacebook-github-bot
authored andcommitted
Fix TaskDispatchThread (#53961)
Summary: Pull Request resolved: #53961 Changelog: [Internal] Fixed a bug in TaskDispatchThread. To understand bug, let me show an example. The task is scheduled to run after several seconds. It reaches loopCv_.wait_until() and waits there. There are 2 bad scenario: 1. wait_until spuriously wakes up and async task runs even before its scheduled time 2. new task is added with runSync(). New task gets added to the queue and loopCv_ is notified. Async task will run before its scheduled time and even worse queue_.pop() will remove sync task. Therefore runSync will be blocked for forever. To fix this bug, we need to add `continue` after wait_until(). Also I added new test to prevent this bug in future. Reviewed By: rshest Differential Revision: D83345047 fbshipit-source-id: 37962613a123a123c0e110426ae782effe5a81c1
1 parent 2d2011c commit ad5949f

File tree

2 files changed

+13
-0
lines changed

2 files changed

+13
-0
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ void TaskDispatchThread::loop() noexcept {
118118
if (task.dispatchTime > now) {
119119
// Wait until the scheduled task time, if delayed
120120
loopCv_.wait_until(lock, task.dispatchTime);
121+
continue;
121122
}
122123
} else {
123124
// Shutting down, skip all the remaining tasks

packages/react-native/ReactCxxPlatform/react/threading/tests/TaskDispatchThreadTests.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,10 +128,12 @@ TEST_F(TaskDispatchThreadTest, RunAsyncFromMultipleThreads) {
128128
EXPECT_EQ(counter.load(), 3);
129129
}
130130

131+
// Test: quit() shouldn't block if it is called inside loop thread
131132
TEST_F(TaskDispatchThreadTest, QuitInTaskShouldntBeBlockedForever) {
132133
dispatcher->runSync([&] { dispatcher->quit(); });
133134
}
134135

136+
// Test: quit() should wait for already running task in the thread
135137
TEST_F(TaskDispatchThreadTest, QuitShouldWaitAlreadyRunningTask) {
136138
{
137139
std::unique_ptr<int> counter = std::make_unique<int>(0);
@@ -147,4 +149,14 @@ TEST_F(TaskDispatchThreadTest, QuitShouldWaitAlreadyRunningTask) {
147149
// forcing dispatcher to join thread
148150
dispatcher.reset();
149151
}
152+
153+
// Test: sync tasks shouldn't be blocked for forever due to delayed task
154+
TEST_F(TaskDispatchThreadTest, SyncTaskShouldntBeBlockedDueToDelayedTask) {
155+
std::atomic<int> counter = 0;
156+
constexpr int kHugeDelay = 100000;
157+
dispatcher->runAsync([&] {}, std::chrono::seconds(kHugeDelay));
158+
std::this_thread::sleep_for(std::chrono::milliseconds(50));
159+
dispatcher->runSync([&] { counter++; });
160+
EXPECT_EQ(counter.load(), 1);
161+
}
150162
} // namespace facebook::react

0 commit comments

Comments
 (0)