FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix sporadic deadlock in SIGUSR1 handler · nodejs/node@1def098 · GitHub

/ node Public

Commit 1def098

Browse files
bnoordhuis
authored and
evanlucas
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 <anna@addaleax.net>
1 parent a4ed7df commit 1def098

2 files changed

Lines changed: 75 additions & 59 deletions

File tree

‎src/atomic-polyfill.h‎

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

‎src/node.cc‎

Lines changed: 75 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;
@@ -180,9 +173,14 @@ static double prog_start_time;
180173
static bool debugger_running;
181174
static uv_async_t dispatch_debug_messages_async;
182175

183-
static node::atomic<Isolate*> node_isolate;
176+
static uv_mutex_t node_isolate_mutex;
177+
static v8::Isolate* node_isolate;
184178
static v8::Platform* default_platform;
185179

180+
#ifdef __POSIX__
181+
static uv_sem_t debug_semaphore;
182+
#endif
183+
186184
static void PrintErrorString(const char* format, ...) {
187185
va_list ap;
188186
va_start(ap, format);
@@ -3703,44 +3701,40 @@ static void EnableDebug(Environment* env) {
37033701

37043702
// Called from an arbitrary thread.
37053703
static void TryStartDebugger() {
3706-
// Call only async signal-safe functions here! Don't retry the exchange,
3707-
// it will deadlock when the thread is interrupted inside a critical section.
3708-
if (auto isolate = node_isolate.exchange(nullptr)) {
3704+
uv_mutex_lock(&node_isolate_mutex);
3705+
if (auto isolate = node_isolate) {
37093706
v8::Debug::DebugBreak(isolate);
37103707
uv_async_send(&dispatch_debug_messages_async);
3711-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
37123708
}
3709+
uv_mutex_unlock(&node_isolate_mutex);
37133710
}
37143711

37153712

37163713
// Called from the main thread.
37173714
static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
3718-
// Synchronize with signal handler, see TryStartDebugger.
3719-
Isolate* isolate;
3720-
do {
3721-
isolate = node_isolate.exchange(nullptr);
3722-
} while (isolate == nullptr);
3715+
uv_mutex_lock(&node_isolate_mutex);
3716+
if (auto isolate = node_isolate) {
3717+
if (debugger_running == false) {
3718+
fprintf(stderr, "Starting debugger agent.\n");
37233719

3724-
if (debugger_running == false) {
3725-
fprintf(stderr, "Starting debugger agent.\n");
3720+
HandleScope scope(isolate);
3721+
Environment* env = Environment::GetCurrent(isolate);
3722+
Context::Scope context_scope(env->context());
37263723

3727-
HandleScope scope(isolate);
3728-
Environment* env = Environment::GetCurrent(isolate);
3729-
Context::Scope context_scope(env->context());
3724+
StartDebug(env, false);
3725+
EnableDebug(env);
3726+
}
37303727

3731-
StartDebug(env, false);
3732-
EnableDebug(env);
3728+
Isolate::Scope isolate_scope(isolate);
3729+
v8::Debug::ProcessDebugMessages(isolate);
37333730
}
3734-
3735-
Isolate::Scope isolate_scope(isolate);
3736-
v8::Debug::ProcessDebugMessages(isolate);
3737-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
3731+
uv_mutex_unlock(&node_isolate_mutex);
37383732
}
37393733

37403734

37413735
#ifdef __POSIX__
37423736
static void EnableDebugSignalHandler(int signo) {
3743-
TryStartDebugger();
3737+
uv_sem_post(&debug_semaphore);
37443738
}
37453739

37463740

@@ -3779,11 +3773,46 @@ void DebugProcess(const FunctionCallbackInfo<Value>& args) {
37793773
}
37803774

37813775

3776+
inline void* DebugSignalThreadMain(void* unused) {
3777+
for (;;) {
3778+
uv_sem_wait(&debug_semaphore);
3779+
TryStartDebugger();
3780+
}
3781+
return nullptr;
3782+
}
3783+
3784+
37823785
static int RegisterDebugSignalHandler() {
3783-
// FIXME(bnoordhuis) Should be per-isolate or per-context, not global.
3786+
// Start a watchdog thread for calling v8::Debug::DebugBreak() because
3787+
// it's not safe to call directly from the signal handler, it can
3788+
// deadlock with the thread it interrupts.
3789+
CHECK_EQ(0, uv_sem_init(&debug_semaphore, 0));
3790+
pthread_attr_t attr;
3791+
CHECK_EQ(0, pthread_attr_init(&attr));
3792+
// Don't shrink the thread's stack on FreeBSD. Said platform decided to
3793+
// follow the pthreads specification to the letter rather than in spirit:
3794+
// https://lists.freebsd.org/pipermail/freebsd-current/2014-March/048885.html
3795+
#ifndef __FreeBSD__
3796+
CHECK_EQ(0, pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN));
3797+
#endif // __FreeBSD__
3798+
CHECK_EQ(0, pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED));
3799+
sigset_t sigmask;
3800+
sigfillset(&sigmask);
3801+
CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, &sigmask));
3802+
pthread_t thread;
3803+
const int err =
3804+
pthread_create(&thread, &attr, DebugSignalThreadMain, nullptr);
3805+
CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, nullptr));
3806+
CHECK_EQ(0, pthread_attr_destroy(&attr));
3807+
if (err != 0) {
3808+
fprintf(stderr, "node[%d]: pthread_create: %s\n", getpid(), strerror(err));
3809+
fflush(stderr);
3810+
// Leave SIGUSR1 blocked. We don't install a signal handler,
3811+
// receiving the signal would terminate the process.
3812+
return -err;
3813+
}
37843814
RegisterSignalHandler(SIGUSR1, EnableDebugSignalHandler);
37853815
// Unblock SIGUSR1. A pending SIGUSR1 signal will now be delivered.
3786-
sigset_t sigmask;
37873816
sigemptyset(&sigmask);
37883817
sigaddset(&sigmask, SIGUSR1);
37893818
CHECK_EQ(0, pthread_sigmask(SIG_UNBLOCK, &sigmask, nullptr));
@@ -4025,6 +4054,8 @@ void Init(int* argc,
40254054
// Make inherited handles noninheritable.
40264055
uv_disable_stdio_inheritance();
40274056

4057+
CHECK_EQ(0, uv_mutex_init(&node_isolate_mutex));
4058+
40284059
// init async debug messages dispatching
40294060
// Main thread uses uv_default_loop
40304061
uv_async_init(uv_default_loop(),
@@ -4307,15 +4338,18 @@ static void StartNodeInstance(void* arg) {
43074338
params.code_event_handler = vTune::GetVtuneCodeEventHandler();
43084339
#endif
43094340
Isolate* isolate = Isolate::New(params);
4341+
4342+
uv_mutex_lock(&node_isolate_mutex);
4343+
if (instance_data->is_main()) {
4344+
CHECK_EQ(node_isolate, nullptr);
4345+
node_isolate = isolate;
4346+
}
4347+
uv_mutex_unlock(&node_isolate_mutex);
4348+
43104349
if (track_heap_objects) {
43114350
isolate->GetHeapProfiler()->StartTrackingHeapObjects(true);
43124351
}
43134352

4314-
// Fetch a reference to the main isolate, so we have a reference to it
4315-
// even when we need it to access it from another (debugger) thread.
4316-
if (instance_data->is_main())
4317-
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
4318-
43194353
{
43204354
Locker locker(isolate);
43214355
Isolate::Scope isolate_scope(isolate);
@@ -4379,10 +4413,10 @@ static void StartNodeInstance(void* arg) {
43794413
env = nullptr;
43804414
}
43814415

4382-
if (instance_data->is_main()) {
4383-
// Synchronize with signal handler, see TryStartDebugger.
4384-
while (isolate != node_isolate.exchange(nullptr)); // NOLINT
4385-
}
4416+
uv_mutex_lock(&node_isolate_mutex);
4417+
if (node_isolate == isolate)
4418+
node_isolate = nullptr;
4419+
uv_mutex_unlock(&node_isolate_mutex);
43864420

43874421
CHECK_NE(isolate, nullptr);
43884422
isolate->Dispose();

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL