Skip to content

Commit 5fb879c

Browse files
joyeecheungnodejs-github-bot
authored andcommitted
src: only block on user blocking worker tasks
we should not be blocking on the worker tasks on the main thread in one go. Doing so leads to two problems: 1. If any of the worker tasks post another foreground task and wait for it to complete, and that foreground task is posted right after we flush the foreground task queue and before the foreground thread goes into sleep, we'll never be able to wake up to execute that foreground task and in turn the worker task will never complete, and we have a deadlock. 2. Worker tasks can be posted from any thread, not necessarily associated with the current isolate, and we can be blocking on a worker task that is associated with a completely unrelated isolate in the event loop. This is suboptimal. However, not blocking on the worker tasks at all can lead to loss of some critical user-blocking worker tasks e.g. wasm async compilation tasks, which should block the main thread until they are completed, as the documentation suggets. As a compromise, we currently only block on user-blocking tasks to reduce the chance of deadlocks while making sure that criticl user-blocking tasks are not lost. PR-URL: #58047 Refs: #47452 Refs: #54918 Reviewed-By: Stephen Belanger <[email protected]>
1 parent fa3c0e0 commit 5fb879c

File tree

2 files changed

+48
-15
lines changed

2 files changed

+48
-15
lines changed

src/node_platform.cc

+42-12
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,10 @@ static void PlatformWorkerThread(void* data) {
7878
fflush(stderr);
7979
}
8080
entry->task->Run();
81-
pending_worker_tasks->Lock().NotifyOfCompletion();
81+
// See NodePlatform::DrainTasks().
82+
if (entry->is_outstanding()) {
83+
pending_worker_tasks->Lock().NotifyOfOutstandingCompletion();
84+
}
8285
}
8386
}
8487

@@ -209,8 +212,10 @@ class WorkerThreadsTaskRunner::DelayedTaskScheduler {
209212
static void RunTask(uv_timer_t* timer) {
210213
DelayedTaskScheduler* scheduler =
211214
ContainerOf(&DelayedTaskScheduler::loop_, timer->loop);
212-
scheduler->pending_worker_tasks_->Lock().Push(
213-
scheduler->TakeTimerTask(timer));
215+
auto entry = scheduler->TakeTimerTask(timer);
216+
bool is_outstanding = entry->is_outstanding();
217+
scheduler->pending_worker_tasks_->Lock().Push(std::move(entry),
218+
is_outstanding);
214219
}
215220

216221
std::unique_ptr<TaskQueueEntry> TakeTimerTask(uv_timer_t* timer) {
@@ -277,7 +282,8 @@ void WorkerThreadsTaskRunner::PostTask(v8::TaskPriority priority,
277282
std::unique_ptr<v8::Task> task,
278283
const v8::SourceLocation& location) {
279284
auto entry = std::make_unique<TaskQueueEntry>(std::move(task), priority);
280-
pending_worker_tasks_.Lock().Push(std::move(entry));
285+
bool is_outstanding = entry->is_outstanding();
286+
pending_worker_tasks_.Lock().Push(std::move(entry), is_outstanding);
281287
}
282288

283289
void WorkerThreadsTaskRunner::PostDelayedTask(
@@ -574,7 +580,25 @@ void NodePlatform::DrainTasks(Isolate* isolate) {
574580
if (!per_isolate) return;
575581

576582
do {
577-
// Worker tasks aren't associated with an Isolate.
583+
// FIXME(54918): we should not be blocking on the worker tasks on the
584+
// main thread in one go. Doing so leads to two problems:
585+
// 1. If any of the worker tasks post another foreground task and wait
586+
// for it to complete, and that foreground task is posted right after
587+
// we flush the foreground task queue and before the foreground thread
588+
// goes into sleep, we'll never be able to wake up to execute that
589+
// foreground task and in turn the worker task will never complete, and
590+
// we have a deadlock.
591+
// 2. Worker tasks can be posted from any thread, not necessarily associated
592+
// with the current isolate, and we can be blocking on a worker task that
593+
// is associated with a completely unrelated isolate in the event loop.
594+
// This is suboptimal.
595+
//
596+
// However, not blocking on the worker tasks at all can lead to loss of some
597+
// critical user-blocking worker tasks e.g. wasm async compilation tasks,
598+
// which should block the main thread until they are completed, as the
599+
// documentation suggets. As a compromise, we currently only block on
600+
// user-blocking tasks to reduce the chance of deadlocks while making sure
601+
// that criticl user-blocking tasks are not lost.
578602
worker_thread_task_runner_->BlockingDrain();
579603
} while (per_isolate->FlushForegroundTasksInternal());
580604
}
@@ -748,16 +772,22 @@ v8::PageAllocator* NodePlatform::GetPageAllocator() {
748772

749773
template <class T>
750774
TaskQueue<T>::TaskQueue()
751-
: lock_(), tasks_available_(), tasks_drained_(),
752-
outstanding_tasks_(0), stopped_(false), task_queue_() { }
775+
: lock_(),
776+
tasks_available_(),
777+
outstanding_tasks_drained_(),
778+
outstanding_tasks_(0),
779+
stopped_(false),
780+
task_queue_() {}
753781

754782
template <class T>
755783
TaskQueue<T>::Locked::Locked(TaskQueue* queue)
756784
: queue_(queue), lock_(queue->lock_) {}
757785

758786
template <class T>
759-
void TaskQueue<T>::Locked::Push(std::unique_ptr<T> task) {
760-
queue_->outstanding_tasks_++;
787+
void TaskQueue<T>::Locked::Push(std::unique_ptr<T> task, bool outstanding) {
788+
if (outstanding) {
789+
queue_->outstanding_tasks_++;
790+
}
761791
queue_->task_queue_.push(std::move(task));
762792
queue_->tasks_available_.Signal(lock_);
763793
}
@@ -788,16 +818,16 @@ std::unique_ptr<T> TaskQueue<T>::Locked::BlockingPop() {
788818
}
789819

790820
template <class T>
791-
void TaskQueue<T>::Locked::NotifyOfCompletion() {
821+
void TaskQueue<T>::Locked::NotifyOfOutstandingCompletion() {
792822
if (--queue_->outstanding_tasks_ == 0) {
793-
queue_->tasks_drained_.Broadcast(lock_);
823+
queue_->outstanding_tasks_drained_.Broadcast(lock_);
794824
}
795825
}
796826

797827
template <class T>
798828
void TaskQueue<T>::Locked::BlockingDrain() {
799829
while (queue_->outstanding_tasks_ > 0) {
800-
queue_->tasks_drained_.Wait(lock_);
830+
queue_->outstanding_tasks_drained_.Wait(lock_);
801831
}
802832
}
803833

src/node_platform.h

+6-3
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,10 @@ class TaskQueue {
4848
EntryCompare>;
4949
class Locked {
5050
public:
51-
void Push(std::unique_ptr<T> task);
51+
void Push(std::unique_ptr<T> task, bool outstanding = false);
5252
std::unique_ptr<T> Pop();
5353
std::unique_ptr<T> BlockingPop();
54-
void NotifyOfCompletion();
54+
void NotifyOfOutstandingCompletion();
5555
void BlockingDrain();
5656
void Stop();
5757
PriorityQueue PopAll();
@@ -72,7 +72,7 @@ class TaskQueue {
7272
private:
7373
Mutex lock_;
7474
ConditionVariable tasks_available_;
75-
ConditionVariable tasks_drained_;
75+
ConditionVariable outstanding_tasks_drained_;
7676
int outstanding_tasks_;
7777
bool stopped_;
7878
PriorityQueue task_queue_;
@@ -83,6 +83,9 @@ struct TaskQueueEntry {
8383
v8::TaskPriority priority;
8484
TaskQueueEntry(std::unique_ptr<v8::Task> t, v8::TaskPriority p)
8585
: task(std::move(t)), priority(p) {}
86+
inline bool is_outstanding() const {
87+
return priority == v8::TaskPriority::kUserBlocking;
88+
}
8689
};
8790

8891
struct DelayedTask {

0 commit comments

Comments
 (0)