Skip to content

Commit

Permalink
worker: fix deadlock when calling terminate from exit handler
Browse files Browse the repository at this point in the history
Just before we call the `'exit'` handlers of a Worker, we drain
the public port’s message queue to ensure proper ordering.
Previously, we held the Worker’s `mutex_` during the
exit handler call, so calling `terminate()` on the worker
could lead to a deadlock if called from one of those message
handlers.

This fixes flakiness in the `parallel/test-worker-dns-terminate` test.

PR-URL: #22073
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and rvagg committed Aug 15, 2018
1 parent 5018661 commit 677d10c
Showing 1 changed file with 12 additions and 12 deletions.
24 changes: 12 additions & 12 deletions src/node_worker.cc
Expand Up @@ -287,22 +287,21 @@ void Worker::JoinThread() {
}

void Worker::OnThreadStopped() {
Mutex::ScopedLock lock(mutex_);
scheduled_on_thread_stopped_ = false;
{
Mutex::ScopedLock lock(mutex_);
scheduled_on_thread_stopped_ = false;

Debug(this, "Worker %llu thread stopped", thread_id_);
Debug(this, "Worker %llu thread stopped", thread_id_);

{
Mutex::ScopedLock stopped_lock(stopped_mutex_);
CHECK(stopped_);
}
{
Mutex::ScopedLock stopped_lock(stopped_mutex_);
CHECK(stopped_);
}

CHECK_EQ(child_port_, nullptr);
parent_port_ = nullptr;
CHECK_EQ(child_port_, nullptr);
parent_port_ = nullptr;
}

// It's okay to join the thread while holding the mutex because
// OnThreadStopped means it's no longer doing any work that might grab it
// and really just silently exiting.
JoinThread();

{
Expand Down Expand Up @@ -369,6 +368,7 @@ void Worker::StopThread(const FunctionCallbackInfo<Value>& args) {
Worker* w;
ASSIGN_OR_RETURN_UNWRAP(&w, args.This());

Debug(w, "Worker %llu is getting stopped by parent", w->thread_id_);
w->Exit(1);
w->JoinThread();
}
Expand Down

0 comments on commit 677d10c

Please sign in to comment.