Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit db2b23f

Browse filesBrowse files
bnoordhuisMyles Borins
authored andcommitted
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 <anna@addaleax.net>
1 parent 8557597 commit db2b23f
Copy full SHA for db2b23f

File tree

Expand file treeCollapse file tree

2 files changed

+74
-59
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+74
-59
lines changed
Open diff view settings
Collapse file

‎src/atomic-polyfill.h‎

Copy file name to clipboardExpand all lines: src/atomic-polyfill.h
-18Lines changed: 0 additions & 18 deletions
This file was deleted.
Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
+74-41Lines 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;
@@ -3538,44 +3535,40 @@ static void EnableDebug(Environment* env) {
35383535

35393536
// Called from an arbitrary thread.
35403537
static void TryStartDebugger() {
3541-
// Call only async signal-safe functions here! Don't retry the exchange,
3542-
// it will deadlock when the thread is interrupted inside a critical section.
3543-
if (auto isolate = node_isolate.exchange(nullptr)) {
3538+
uv_mutex_lock(&node_isolate_mutex);
3539+
if (auto isolate = node_isolate) {
35443540
v8::Debug::DebugBreak(isolate);
35453541
uv_async_send(&dispatch_debug_messages_async);
3546-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
35473542
}
3543+
uv_mutex_unlock(&node_isolate_mutex);
35483544
}
35493545

35503546

35513547
// Called from the main thread.
35523548
static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
3553-
// Synchronize with signal handler, see TryStartDebugger.
3554-
Isolate* isolate;
3555-
do {
3556-
isolate = node_isolate.exchange(nullptr);
3557-
} while (isolate == nullptr);
3549+
uv_mutex_lock(&node_isolate_mutex);
3550+
if (auto isolate = node_isolate) {
3551+
if (debugger_running == false) {
3552+
fprintf(stderr, "Starting debugger agent.\n");
35583553

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

3562-
HandleScope scope(isolate);
3563-
Environment* env = Environment::GetCurrent(isolate);
3564-
Context::Scope context_scope(env->context());
3558+
StartDebug(env, false);
3559+
EnableDebug(env);
3560+
}
35653561

3566-
StartDebug(env, false);
3567-
EnableDebug(env);
3562+
Isolate::Scope isolate_scope(isolate);
3563+
v8::Debug::ProcessDebugMessages();
35683564
}
3569-
3570-
Isolate::Scope isolate_scope(isolate);
3571-
v8::Debug::ProcessDebugMessages();
3572-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
3565+
uv_mutex_unlock(&node_isolate_mutex);
35733566
}
35743567

35753568

35763569
#ifdef __POSIX__
35773570
static void EnableDebugSignalHandler(int signo) {
3578-
TryStartDebugger();
3571+
uv_sem_post(&debug_semaphore);
35793572
}
35803573

35813574

@@ -3614,11 +3607,46 @@ void DebugProcess(const FunctionCallbackInfo<Value>& args) {
36143607
}
36153608

36163609

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

3891+
CHECK_EQ(0, uv_mutex_init(&node_isolate_mutex));
3892+
38633893
// init async debug messages dispatching
38643894
// Main thread uses uv_default_loop
38653895
CHECK_EQ(0, uv_async_init(uv_default_loop(),
@@ -4146,15 +4176,18 @@ static void StartNodeInstance(void* arg) {
41464176
params.code_event_handler = vTune::GetVtuneCodeEventHandler();
41474177
#endif
41484178
Isolate* isolate = Isolate::New(params);
4179+
4180+
uv_mutex_lock(&node_isolate_mutex);
4181+
if (instance_data->is_main()) {
4182+
CHECK_EQ(node_isolate, nullptr);
4183+
node_isolate = isolate;
4184+
}
4185+
uv_mutex_unlock(&node_isolate_mutex);
4186+
41494187
if (track_heap_objects) {
41504188
isolate->GetHeapProfiler()->StartTrackingHeapObjects(true);
41514189
}
41524190

4153-
// Fetch a reference to the main isolate, so we have a reference to it
4154-
// even when we need it to access it from another (debugger) thread.
4155-
if (instance_data->is_main())
4156-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
4157-
41584191
{
41594192
Locker locker(isolate);
41604193
Isolate::Scope isolate_scope(isolate);
@@ -4218,10 +4251,10 @@ static void StartNodeInstance(void* arg) {
42184251
env = nullptr;
42194252
}
42204253

4221-
if (instance_data->is_main()) {
4222-
// Synchronize with signal handler, see TryStartDebugger.
4223-
while (isolate != node_isolate.exchange(nullptr)); // NOLINT
4224-
}
4254+
uv_mutex_lock(&node_isolate_mutex);
4255+
if (node_isolate == isolate)
4256+
node_isolate = nullptr;
4257+
uv_mutex_unlock(&node_isolate_mutex);
42254258

42264259
CHECK_NE(isolate, nullptr);
42274260
isolate->Dispose();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.