Skip to content

Commit 2b81f80

Browse files
committed
src: fix sporadic deadlock in SIGUSR1 handler
Calling v8::Debug::DebugBreak() directly from the signal handler is unsafe because said function tries to grab a mutex. Work around that by starting a watchdog thread that is woken up from the signal handler, which then calls v8::Debug::DebugBreak(). Using a watchdog thread also removes the need to use atomic operations so this commit does away with that. Fixes: #5368 PR-URL: #5904 Reviewed-By: Anna Henningsen <[email protected]>
1 parent 110ce55 commit 2b81f80

File tree

2 files changed

+74
-59
lines changed

2 files changed

+74
-59
lines changed

src/atomic-polyfill.h

Lines changed: 0 additions & 18 deletions
This file was deleted.

src/node.cc

Lines changed: 74 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
#define umask _umask
7474
typedef int mode_t;
7575
#else
76+
#include <pthread.h>
7677
#include <sys/resource.h> // getrlimit, setrlimit
7778
#include <unistd.h> // setuid, getuid
7879
#endif
@@ -89,14 +90,6 @@ typedef int mode_t;
8990
extern char **environ;
9091
#endif
9192

92-
#ifdef __APPLE__
93-
#include "atomic-polyfill.h" // NOLINT(build/include_order)
94-
namespace node { template <typename T> using atomic = nonstd::atomic<T>; }
95-
#else
96-
#include <atomic>
97-
namespace node { template <typename T> using atomic = std::atomic<T>; }
98-
#endif
99-
10093
namespace node {
10194

10295
using v8::Array;
@@ -166,9 +159,13 @@ static double prog_start_time;
166159
static bool debugger_running;
167160
static uv_async_t dispatch_debug_messages_async;
168161

169-
static node::atomic<Isolate*> node_isolate;
162+
static uv_mutex_t node_isolate_mutex;
163+
static v8::Isolate* node_isolate;
170164
static v8::Platform* default_platform;
171165

166+
#ifdef __POSIX__
167+
static uv_sem_t debug_semaphore;
168+
#endif
172169

173170
static void PrintErrorString(const char* format, ...) {
174171
va_list ap;
@@ -3533,44 +3530,40 @@ static void EnableDebug(Environment* env) {
35333530

35343531
// Called from an arbitrary thread.
35353532
static void TryStartDebugger() {
3536-
// Call only async signal-safe functions here! Don't retry the exchange,
3537-
// it will deadlock when the thread is interrupted inside a critical section.
3538-
if (auto isolate = node_isolate.exchange(nullptr)) {
3533+
uv_mutex_lock(&node_isolate_mutex);
3534+
if (auto isolate = node_isolate) {
35393535
v8::Debug::DebugBreak(isolate);
35403536
uv_async_send(&dispatch_debug_messages_async);
3541-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
35423537
}
3538+
uv_mutex_unlock(&node_isolate_mutex);
35433539
}
35443540

35453541

35463542
// Called from the main thread.
35473543
static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
3548-
// Synchronize with signal handler, see TryStartDebugger.
3549-
Isolate* isolate;
3550-
do {
3551-
isolate = node_isolate.exchange(nullptr);
3552-
} while (isolate == nullptr);
3544+
uv_mutex_lock(&node_isolate_mutex);
3545+
if (auto isolate = node_isolate) {
3546+
if (debugger_running == false) {
3547+
fprintf(stderr, "Starting debugger agent.\n");
35533548

3554-
if (debugger_running == false) {
3555-
fprintf(stderr, "Starting debugger agent.\n");
3549+
HandleScope scope(isolate);
3550+
Environment* env = Environment::GetCurrent(isolate);
3551+
Context::Scope context_scope(env->context());
35563552

3557-
HandleScope scope(isolate);
3558-
Environment* env = Environment::GetCurrent(isolate);
3559-
Context::Scope context_scope(env->context());
3553+
StartDebug(env, false);
3554+
EnableDebug(env);
3555+
}
35603556

3561-
StartDebug(env, false);
3562-
EnableDebug(env);
3557+
Isolate::Scope isolate_scope(isolate);
3558+
v8::Debug::ProcessDebugMessages();
35633559
}
3564-
3565-
Isolate::Scope isolate_scope(isolate);
3566-
v8::Debug::ProcessDebugMessages();
3567-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
3560+
uv_mutex_unlock(&node_isolate_mutex);
35683561
}
35693562

35703563

35713564
#ifdef __POSIX__
35723565
static void EnableDebugSignalHandler(int signo) {
3573-
TryStartDebugger();
3566+
uv_sem_post(&debug_semaphore);
35743567
}
35753568

35763569

@@ -3609,11 +3602,46 @@ void DebugProcess(const FunctionCallbackInfo<Value>& args) {
36093602
}
36103603

36113604

3605+
inline void* DebugSignalThreadMain(void* unused) {
3606+
for (;;) {
3607+
uv_sem_wait(&debug_semaphore);
3608+
TryStartDebugger();
3609+
}
3610+
return nullptr;
3611+
}
3612+
3613+
36123614
static int RegisterDebugSignalHandler() {
3613-
// FIXME(bnoordhuis) Should be per-isolate or per-context, not global.
3615+
// Start a watchdog thread for calling v8::Debug::DebugBreak() because
3616+
// it's not safe to call directly from the signal handler, it can
3617+
// deadlock with the thread it interrupts.
3618+
CHECK_EQ(0, uv_sem_init(&debug_semaphore, 0));
3619+
pthread_attr_t attr;
3620+
CHECK_EQ(0, pthread_attr_init(&attr));
3621+
// Don't shrink the thread's stack on FreeBSD. Said platform decided to
3622+
// follow the pthreads specification to the letter rather than in spirit:
3623+
// https://lists.freebsd.org/pipermail/freebsd-current/2014-March/048885.html
3624+
#ifndef __FreeBSD__
3625+
CHECK_EQ(0, pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN));
3626+
#endif // __FreeBSD__
3627+
CHECK_EQ(0, pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED));
3628+
sigset_t sigmask;
3629+
sigfillset(&sigmask);
3630+
CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, &sigmask));
3631+
pthread_t thread;
3632+
const int err =
3633+
pthread_create(&thread, &attr, DebugSignalThreadMain, nullptr);
3634+
CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, nullptr));
3635+
CHECK_EQ(0, pthread_attr_destroy(&attr));
3636+
if (err != 0) {
3637+
fprintf(stderr, "node[%d]: pthread_create: %s\n", getpid(), strerror(err));
3638+
fflush(stderr);
3639+
// Leave SIGUSR1 blocked. We don't install a signal handler,
3640+
// receiving the signal would terminate the process.
3641+
return -err;
3642+
}
36143643
RegisterSignalHandler(SIGUSR1, EnableDebugSignalHandler);
36153644
// Unblock SIGUSR1. A pending SIGUSR1 signal will now be delivered.
3616-
sigset_t sigmask;
36173645
sigemptyset(&sigmask);
36183646
sigaddset(&sigmask, SIGUSR1);
36193647
CHECK_EQ(0, pthread_sigmask(SIG_UNBLOCK, &sigmask, nullptr));
@@ -3855,6 +3883,8 @@ void Init(int* argc,
38553883
// Make inherited handles noninheritable.
38563884
uv_disable_stdio_inheritance();
38573885

3886+
CHECK_EQ(0, uv_mutex_init(&node_isolate_mutex));
3887+
38583888
// init async debug messages dispatching
38593889
// Main thread uses uv_default_loop
38603890
CHECK_EQ(0, uv_async_init(uv_default_loop(),
@@ -4141,15 +4171,18 @@ static void StartNodeInstance(void* arg) {
41414171
params.code_event_handler = vTune::GetVtuneCodeEventHandler();
41424172
#endif
41434173
Isolate* isolate = Isolate::New(params);
4174+
4175+
uv_mutex_lock(&node_isolate_mutex);
4176+
if (instance_data->is_main()) {
4177+
CHECK_EQ(node_isolate, nullptr);
4178+
node_isolate = isolate;
4179+
}
4180+
uv_mutex_unlock(&node_isolate_mutex);
4181+
41444182
if (track_heap_objects) {
41454183
isolate->GetHeapProfiler()->StartTrackingHeapObjects(true);
41464184
}
41474185

4148-
// Fetch a reference to the main isolate, so we have a reference to it
4149-
// even when we need it to access it from another (debugger) thread.
4150-
if (instance_data->is_main())
4151-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
4152-
41534186
{
41544187
Locker locker(isolate);
41554188
Isolate::Scope isolate_scope(isolate);
@@ -4213,10 +4246,10 @@ static void StartNodeInstance(void* arg) {
42134246
env = nullptr;
42144247
}
42154248

4216-
if (instance_data->is_main()) {
4217-
// Synchronize with signal handler, see TryStartDebugger.
4218-
while (isolate != node_isolate.exchange(nullptr)); // NOLINT
4219-
}
4249+
uv_mutex_lock(&node_isolate_mutex);
4250+
if (node_isolate == isolate)
4251+
node_isolate = nullptr;
4252+
uv_mutex_unlock(&node_isolate_mutex);
42204253

42214254
CHECK_NE(isolate, nullptr);
42224255
isolate->Dispose();

0 commit comments

Comments
 (0)