Skip to content

Commit ca363cf

Browse files
bnoordhuisbnoordhuis
committed
debugger: make busy loops SIGUSR1-interruptible
Commit 30e5366 ("core: Use a uv_signal for debug listener") changed SIGUSR1 handling from a signal handler to libuv's uv_signal_*() functionality to fix a race condition (and possible hang) in the signal handler. While a good change in itself, it made it impossible to interrupt long running scripts. When a script is stuck in a busy loop, control never returns to the event loop, which in turn means the signal callback - and therefore the debugger - is never invoked. This commit changes SIGUSR1 handling back to a normal signal handler but one that treads _very_ carefully.
1 parent 085dd30 commit ca363cf

6 files changed

Lines changed: 64 additions & 109 deletions

File tree

‎src/node.cc‎

Lines changed: 48 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -138,10 +138,8 @@ bool no_deprecation = false;
138138

139139
// process-relative uptime base, initialized at start-up
140140
static double prog_start_time;
141-
142-
static volatile bool debugger_running = false;
141+
static bool debugger_running;
143142
static uv_async_t dispatch_debug_messages_async;
144-
static uv_async_t emit_debug_enabled_async;
145143

146144
// Declared in node_internals.h
147145
Isolate* node_isolate = NULL;
@@ -2823,85 +2821,68 @@ static void ParseArgs(int* argc,
28232821
}
28242822

28252823

2826-
// Called from the main thread.
2827-
static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle, int status) {
2828-
v8::Debug::ProcessDebugMessages();
2829-
}
2830-
2831-
28322824
// Called from V8 Debug Agent TCP thread.
28332825
static void DispatchMessagesDebugAgentCallback() {
28342826
uv_async_send(&dispatch_debug_messages_async);
28352827
}
28362828

28372829

2838-
// Called from the main thread
2839-
static void EmitDebugEnabledAsyncCallback(uv_async_t* handle, int status) {
2840-
Environment* env = Environment::GetCurrent(node_isolate);
2830+
// Called from the main thread.
2831+
static void EnableDebug(bool wait_connect) {
2832+
assert(debugger_running == false);
2833+
Isolate* isolate = node_isolate; // TODO(bnoordhuis) Multi-isolate support.
2834+
Isolate::Scope isolate_scope(isolate);
2835+
v8::Debug::SetDebugMessageDispatchHandler(DispatchMessagesDebugAgentCallback,
2836+
false);
2837+
debugger_running = v8::Debug::EnableAgent("node " NODE_VERSION,
2838+
debug_port,
2839+
wait_connect);
2840+
if (debugger_running == false) {
2841+
fprintf(stderr, "Starting debugger on port %d failed\n", debug_port);
2842+
fflush(stderr);
2843+
return;
2844+
}
2845+
fprintf(stderr, "Debugger listening on port %d\n", debug_port);
2846+
fflush(stderr);
2847+
2848+
Environment* env = Environment::GetCurrentChecked(isolate);
2849+
if (env == NULL)
2850+
return; // Still starting up.
2851+
28412852
Context::Scope context_scope(env->context());
28422853
HandleScope handle_scope(env->isolate());
28432854
Local<Object> message = Object::New();
2844-
message->Set(FIXED_ONE_BYTE_STRING(node_isolate, "cmd"),
2845-
FIXED_ONE_BYTE_STRING(node_isolate, "NODE_DEBUG_ENABLED"));
2846-
Local<Value> args[] = {
2847-
FIXED_ONE_BYTE_STRING(node_isolate, "internalMessage"),
2855+
message->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "cmd"),
2856+
FIXED_ONE_BYTE_STRING(env->isolate(), "NODE_DEBUG_ENABLED"));
2857+
Local<Value> argv[] = {
2858+
FIXED_ONE_BYTE_STRING(env->isolate(), "internalMessage"),
28482859
message
28492860
};
2850-
MakeCallback(env, env->process_object(), "emit", ARRAY_SIZE(args), args);
2851-
}
2852-
2853-
2854-
// Called from the signal watcher callback
2855-
static void EmitDebugEnabled() {
2856-
uv_async_send(&emit_debug_enabled_async);
2861+
MakeCallback(env, env->process_object(), "emit", ARRAY_SIZE(argv), argv);
28572862
}
28582863

28592864

2860-
static void EnableDebug(bool wait_connect) {
2861-
// If we're called from another thread, make sure to enter the right
2862-
// v8 isolate.
2863-
node_isolate->Enter();
2864-
2865-
v8::Debug::SetDebugMessageDispatchHandler(DispatchMessagesDebugAgentCallback,
2866-
false);
2867-
2868-
// Start the debug thread and it's associated TCP server on port 5858.
2869-
bool r = v8::Debug::EnableAgent("node " NODE_VERSION,
2870-
debug_port,
2871-
wait_connect);
2872-
2873-
// Crappy check that everything went well. FIXME
2874-
assert(r);
2875-
2876-
// Print out some information.
2877-
fprintf(stderr, "debugger listening on port %d\n", debug_port);
2878-
fflush(stderr);
2879-
2880-
debugger_running = true;
2881-
2882-
if (Environment::GetCurrentChecked(node_isolate) != NULL) {
2883-
EmitDebugEnabled();
2865+
// Called from the main thread.
2866+
static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle, int status) {
2867+
if (debugger_running == false) {
2868+
fprintf(stderr, "Starting debugger agent.\n");
2869+
EnableDebug(false);
28842870
}
2885-
2886-
node_isolate->Exit();
2871+
Isolate::Scope isolate_scope(node_isolate);
2872+
v8::Debug::ProcessDebugMessages();
28872873
}
28882874

28892875

28902876
#ifdef __POSIX__
2891-
static void EnableDebugSignalHandler(uv_signal_t* handle, int) {
2892-
// Break once process will return execution to v8
2893-
v8::Debug::DebugBreak(node_isolate);
2894-
2895-
if (!debugger_running) {
2896-
fprintf(stderr, "Hit SIGUSR1 - starting debugger agent.\n");
2897-
EnableDebug(false);
2898-
}
2877+
static void EnableDebugSignalHandler(int signo) {
2878+
// Call only async signal-safe functions here!
2879+
v8::Debug::DebugBreak(*static_cast<Isolate* volatile*>(&node_isolate));
2880+
uv_async_send(&dispatch_debug_messages_async);
28992881
}
29002882

29012883

29022884
static void RegisterSignalHandler(int signal, void (*handler)(int signal)) {
29032885
struct sigaction sa;
2904-
29052886
memset(&sa, 0, sizeof(sa));
29062887
sa.sa_handler = handler;
29072888
sigfillset(&sa.sa_mask);
@@ -2925,22 +2906,20 @@ void DebugProcess(const FunctionCallbackInfo<Value>& args) {
29252906
return ThrowErrnoException(errno, "kill");
29262907
}
29272908
}
2909+
2910+
2911+
static int RegisterDebugSignalHandler() {
2912+
// FIXME(bnoordhuis) Should be per-isolate or per-context, not global.
2913+
RegisterSignalHandler(SIGUSR1, EnableDebugSignalHandler);
2914+
return 0;
2915+
}
29282916
#endif // __POSIX__
29292917

29302918

29312919
#ifdef _WIN32
29322920
DWORD WINAPI EnableDebugThreadProc(void* arg) {
2933-
// Break once process will return execution to v8
2934-
if (!debugger_running) {
2935-
for (int i = 0; i < 1; i++) {
2936-
fprintf(stderr, "Starting debugger agent.\r\n");
2937-
fflush(stderr);
2938-
EnableDebug(false);
2939-
}
2940-
}
2941-
2942-
v8::Debug::DebugBreak(node_isolate);
2943-
2921+
v8::Debug::DebugBreak(*static_cast<Isolate* volatile*>(&node_isolate));
2922+
uv_async_send(&dispatch_debug_messages_async);
29442923
return 0;
29452924
}
29462925

@@ -3105,13 +3084,6 @@ void Init(int* argc,
31053084
DispatchDebugMessagesAsyncCallback);
31063085
uv_unref(reinterpret_cast<uv_handle_t*>(&dispatch_debug_messages_async));
31073086

3108-
// init async NODE_DEBUG_ENABLED emitter
3109-
// FIXME(bnoordhuis) Should be per-isolate or per-context, not global.
3110-
uv_async_init(uv_default_loop(),
3111-
&emit_debug_enabled_async,
3112-
EmitDebugEnabledAsyncCallback);
3113-
uv_unref(reinterpret_cast<uv_handle_t*>(&emit_debug_enabled_async));
3114-
31153087
// Parse a few arguments which are specific to Node.
31163088
int v8_argc;
31173089
const char** v8_argv;
@@ -3192,15 +3164,7 @@ void Init(int* argc,
31923164
if (use_debug_agent) {
31933165
EnableDebug(debug_wait_connect);
31943166
} else {
3195-
#ifdef _WIN32
31963167
RegisterDebugSignalHandler();
3197-
#else // Posix
3198-
// FIXME(bnoordhuis) Should be per-isolate or per-context, not global.
3199-
static uv_signal_t signal_watcher;
3200-
uv_signal_init(uv_default_loop(), &signal_watcher);
3201-
uv_signal_start(&signal_watcher, EnableDebugSignalHandler, SIGUSR1);
3202-
uv_unref(reinterpret_cast<uv_handle_t*>(&signal_watcher));
3203-
#endif // __POSIX__
32043168
}
32053169
}
32063170

‎test/simple/test-debug-cluster.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,9 @@ child.stderr.on('data', function(data) {
4242

4343
var assertOutputLines = common.mustCall(function() {
4444
var expectedLines = [
45-
'debugger listening on port ' + 5858,
46-
'debugger listening on port ' + 5859,
47-
'debugger listening on port ' + 5860,
45+
'Debugger listening on port ' + 5858,
46+
'Debugger listening on port ' + 5859,
47+
'Debugger listening on port ' + 5860,
4848
];
4949

5050
// Do not assume any particular order of output messages,

‎test/simple/test-debug-port-cluster.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,9 @@ child.stderr.on('data', function(data) {
4848

4949
var assertOutputLines = common.mustCall(function() {
5050
var expectedLines = [
51-
'debugger listening on port ' + port,
52-
'debugger listening on port ' + (port+1),
53-
'debugger listening on port ' + (port+2),
51+
'Debugger listening on port ' + port,
52+
'Debugger listening on port ' + (port+1),
53+
'Debugger listening on port ' + (port+2),
5454
];
5555

5656
// Do not assume any particular order of output messages,

‎test/simple/test-debug-port-from-cmdline.js‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,20 +51,16 @@ function processStderrLine(line) {
5151
console.log('> ' + line);
5252
outputLines.push(line);
5353

54-
if (/debugger listening/.test(line)) {
54+
if (/Debugger listening/.test(line)) {
5555
assertOutputLines();
5656
process.exit();
5757
}
5858
}
5959

6060
function assertOutputLines() {
61-
var startLog = process.platform === 'win32'
62-
? 'Starting debugger agent.'
63-
: 'Hit SIGUSR1 - starting debugger agent.';
64-
6561
var expectedLines = [
66-
startLog,
67-
'debugger listening on port ' + debugPort
62+
'Starting debugger agent.',
63+
'Debugger listening on port ' + debugPort
6864
];
6965

7066
assert.equal(outputLines.length, expectedLines.length);

‎test/simple/test-debug-signal-cluster.js‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -61,17 +61,13 @@ process.on('exit', function onExit() {
6161
});
6262

6363
function assertOutputLines() {
64-
var startLog = process.platform === 'win32'
65-
? 'Starting debugger agent.'
66-
: 'Hit SIGUSR1 - starting debugger agent.';
67-
6864
var expectedLines = [
69-
startLog,
70-
'debugger listening on port ' + 5858,
71-
startLog,
72-
'debugger listening on port ' + 5859,
73-
startLog,
74-
'debugger listening on port ' + 5860,
65+
'Starting debugger agent.',
66+
'Debugger listening on port ' + 5858,
67+
'Starting debugger agent.',
68+
'Debugger listening on port ' + 5859,
69+
'Starting debugger agent.',
70+
'Debugger listening on port ' + 5860,
7571
];
7672

7773
// Do not assume any particular order of output messages,

‎test/simple/test-debugger-client.js‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ function doTest(cb, done) {
182182
nodeProcess.stderr.resume();
183183
b += data;
184184
if (didTryConnect == false &&
185-
b.match(/debugger listening on port/)) {
185+
b.match(/Debugger listening on port/)) {
186186
didTryConnect = true;
187187

188188
setTimeout(function() {
@@ -224,4 +224,3 @@ process.on('exit', function(code) {
224224
if (!code)
225225
assert.equal(expectedConnections, connectCount);
226226
});
227-

0 commit comments

Comments
 (0)