Skip to content

Commit ad5aabb

Browse files
committed
src: fix deadlock in NodePlatform::DrainTasks on process shutdown
Replace the unconditional BlockingDrain() in DrainTasks() with a 1ms timed drain loop that periodically flushes foreground tasks while waiting for outstanding kUserBlocking worker tasks to complete. This fixes a deadlock where a kUserBlocking worker task (e.g. Maglev JIT compilation) posts a foreground task and waits for it. If the foreground task is posted after the main thread flushes the foreground queue but before BlockingDrain() enters its indefinite sleep, both threads wait on each other — a mutual deadlock. With the timed drain, the main thread wakes every 1ms and flushes any foreground tasks that were posted in that window. In the common case, tasks complete before the timeout and the wait is woken immediately by NotifyOfOutstandingCompletion(), so there is no performance regression. Adds ConditionVariableBase::TimedWait() backed by uv_cond_timedwait() and TaskQueue::Locked::TimedBlockingDrain() to support this. Fixes: #54918
1 parent ad945c5 commit ad5aabb

File tree

3 files changed

+61
-20
lines changed

3 files changed

+61
-20
lines changed

src/node_mutex.h

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,9 @@ class ConditionVariableBase {
139139
inline void Broadcast(const ScopedLock&);
140140
inline void Signal(const ScopedLock&);
141141
inline void Wait(const ScopedLock& scoped_lock);
142+
// Returns true if signaled before timeout, false if timed out.
143+
// timeout_ns is in nanoseconds.
144+
inline bool TimedWait(const ScopedLock& scoped_lock, uint64_t timeout_ns);
142145

143146
ConditionVariableBase(const ConditionVariableBase&) = delete;
144147
ConditionVariableBase& operator=(const ConditionVariableBase&) = delete;
@@ -175,6 +178,11 @@ struct LibuvMutexTraits {
175178
uv_cond_wait(cond, mutex);
176179
}
177180

181+
static inline int cond_timedwait(CondT* cond, MutexT* mutex,
182+
uint64_t timeout) {
183+
return uv_cond_timedwait(cond, mutex, timeout);
184+
}
185+
178186
static inline void mutex_destroy(MutexT* mutex) {
179187
uv_mutex_destroy(mutex);
180188
}
@@ -249,6 +257,13 @@ void ConditionVariableBase<Traits>::Wait(const ScopedLock& scoped_lock) {
249257
Traits::cond_wait(&cond_, &scoped_lock.mutex_.mutex_);
250258
}
251259

260+
template <typename Traits>
261+
bool ConditionVariableBase<Traits>::TimedWait(const ScopedLock& scoped_lock,
262+
uint64_t timeout_ns) {
263+
return Traits::cond_timedwait(
264+
&cond_, &scoped_lock.mutex_.mutex_, timeout_ns) == 0;
265+
}
266+
252267
template <typename Traits>
253268
MutexBase<Traits>::MutexBase() {
254269
CHECK_EQ(0, Traits::mutex_init(&mutex_));

src/node_platform.cc

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,10 @@ void WorkerThreadsTaskRunner::BlockingDrain() {
300300
pending_worker_tasks_.Lock().BlockingDrain();
301301
}
302302

303+
bool WorkerThreadsTaskRunner::TimedBlockingDrain(uint64_t timeout_ns) {
304+
return pending_worker_tasks_.Lock().TimedBlockingDrain(timeout_ns);
305+
}
306+
303307
void WorkerThreadsTaskRunner::Shutdown() {
304308
pending_worker_tasks_.Lock().Stop();
305309
delayed_task_scheduler_->Stop();
@@ -580,27 +584,33 @@ void NodePlatform::DrainTasks(Isolate* isolate) {
580584
std::shared_ptr<PerIsolatePlatformData> per_isolate = ForNodeIsolate(isolate);
581585
if (!per_isolate) return;
582586

587+
// Use a timed drain instead of blocking indefinitely on outstanding worker
588+
// tasks. This fixes the deadlock described in:
589+
// https://github.com/nodejs/node/issues/54918
590+
//
591+
// A kUserBlocking worker task (e.g. Maglev JIT compilation) may post a
592+
// foreground task and wait for it to complete. If that foreground task is
593+
// posted after we flush the foreground queue but before we enter the wait
594+
// inside BlockingDrain(), the main thread sleeps while the worker is also
595+
// waiting for its foreground task — a mutual deadlock.
596+
//
597+
// With a timed drain, the main thread wakes up every kDrainIntervalNs and
598+
// flushes any foreground tasks that were posted in that window, allowing
599+
// the blocked worker task to proceed. In the common case, tasks complete
600+
// before the timeout and NotifyOfOutstandingCompletion() wakes the wait
601+
// immediately, so there is no performance regression.
602+
//
603+
// We still wait only on kUserBlocking tasks (tracked via is_outstanding())
604+
// because they are documented to require completion before execution
605+
// continues (e.g. wasm async compilation).
606+
static constexpr uint64_t kDrainIntervalNs = 1'000'000; // 1ms in nanoseconds
607+
583608
do {
584-
// FIXME(54918): we should not be blocking on the worker tasks on the
585-
// main thread in one go. Doing so leads to two problems:
586-
// 1. If any of the worker tasks post another foreground task and wait
587-
// for it to complete, and that foreground task is posted right after
588-
// we flush the foreground task queue and before the foreground thread
589-
// goes into sleep, we'll never be able to wake up to execute that
590-
// foreground task and in turn the worker task will never complete, and
591-
// we have a deadlock.
592-
// 2. Worker tasks can be posted from any thread, not necessarily associated
593-
// with the current isolate, and we can be blocking on a worker task that
594-
// is associated with a completely unrelated isolate in the event loop.
595-
// This is suboptimal.
596-
//
597-
// However, not blocking on the worker tasks at all can lead to loss of some
598-
// critical user-blocking worker tasks e.g. wasm async compilation tasks,
599-
// which should block the main thread until they are completed, as the
600-
// documentation suggets. As a compromise, we currently only block on
601-
// user-blocking tasks to reduce the chance of deadlocks while making sure
602-
// that criticl user-blocking tasks are not lost.
603-
worker_thread_task_runner_->BlockingDrain();
609+
while (!worker_thread_task_runner_->TimedBlockingDrain(kDrainIntervalNs)) {
610+
// Timed out: a worker task may have posted a foreground task and is
611+
// waiting for it. Flush the foreground queue now so it can proceed.
612+
per_isolate->FlushForegroundTasksInternal();
613+
}
604614
} while (per_isolate->FlushForegroundTasksInternal());
605615
}
606616

@@ -832,6 +842,16 @@ void TaskQueue<T>::Locked::BlockingDrain() {
832842
}
833843
}
834844

845+
template <class T>
846+
bool TaskQueue<T>::Locked::TimedBlockingDrain(uint64_t timeout_ns) {
847+
while (queue_->outstanding_tasks_ > 0) {
848+
if (!queue_->outstanding_tasks_drained_.TimedWait(lock_, timeout_ns)) {
849+
return false; // timed out, outstanding tasks still pending
850+
}
851+
}
852+
return true;
853+
}
854+
835855
template <class T>
836856
void TaskQueue<T>::Locked::Stop() {
837857
queue_->stopped_ = true;

src/node_platform.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,9 @@ class TaskQueue {
5353
std::unique_ptr<T> BlockingPop();
5454
void NotifyOfOutstandingCompletion();
5555
void BlockingDrain();
56+
// Returns true if all outstanding tasks completed before the timeout,
57+
// false if timed out. timeout_ns is in nanoseconds.
58+
bool TimedBlockingDrain(uint64_t timeout_ns);
5659
void Stop();
5760
PriorityQueue PopAll();
5861

@@ -196,6 +199,9 @@ class WorkerThreadsTaskRunner {
196199
double delay_in_seconds);
197200

198201
void BlockingDrain();
202+
// Returns true if all outstanding tasks completed before the timeout,
203+
// false if timed out. timeout_ns is in nanoseconds.
204+
bool TimedBlockingDrain(uint64_t timeout_ns);
199205
void Shutdown();
200206

201207
int NumberOfWorkerThreads() const;

0 commit comments

Comments
 (0)