From: Ben Noordhuis Date: Tue, 15 Oct 2013 21:32:18 +0000 (+0200) Subject: debugger: make busy loops SIGUSR1-interruptible X-Git-Tag: v0.11.8~29 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=ca363cf1ae9631cd6465c3a0028ca0af823a68fb;p=platform%2Fupstream%2Fnodejs.git debugger: make busy loops SIGUSR1-interruptible Commit 30e5366b ("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. --- diff --git a/src/node.cc b/src/node.cc index bea3380..2c874ce 100644 --- a/src/node.cc +++ b/src/node.cc @@ -138,10 +138,8 @@ bool no_deprecation = false; // process-relative uptime base, initialized at start-up static double prog_start_time; - -static volatile bool debugger_running = false; +static bool debugger_running; static uv_async_t dispatch_debug_messages_async; -static uv_async_t emit_debug_enabled_async; // Declared in node_internals.h Isolate* node_isolate = NULL; @@ -2823,85 +2821,68 @@ static void ParseArgs(int* argc, } -// Called from the main thread. -static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle, int status) { - v8::Debug::ProcessDebugMessages(); -} - - // Called from V8 Debug Agent TCP thread. static void DispatchMessagesDebugAgentCallback() { uv_async_send(&dispatch_debug_messages_async); } -// Called from the main thread -static void EmitDebugEnabledAsyncCallback(uv_async_t* handle, int status) { - Environment* env = Environment::GetCurrent(node_isolate); +// Called from the main thread. +static void EnableDebug(bool wait_connect) { + assert(debugger_running == false); + Isolate* isolate = node_isolate; // TODO(bnoordhuis) Multi-isolate support. + Isolate::Scope isolate_scope(isolate); + v8::Debug::SetDebugMessageDispatchHandler(DispatchMessagesDebugAgentCallback, + false); + debugger_running = v8::Debug::EnableAgent("node " NODE_VERSION, + debug_port, + wait_connect); + if (debugger_running == false) { + fprintf(stderr, "Starting debugger on port %d failed\n", debug_port); + fflush(stderr); + return; + } + fprintf(stderr, "Debugger listening on port %d\n", debug_port); + fflush(stderr); + + Environment* env = Environment::GetCurrentChecked(isolate); + if (env == NULL) + return; // Still starting up. + Context::Scope context_scope(env->context()); HandleScope handle_scope(env->isolate()); Local message = Object::New(); - message->Set(FIXED_ONE_BYTE_STRING(node_isolate, "cmd"), - FIXED_ONE_BYTE_STRING(node_isolate, "NODE_DEBUG_ENABLED")); - Local args[] = { - FIXED_ONE_BYTE_STRING(node_isolate, "internalMessage"), + message->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "cmd"), + FIXED_ONE_BYTE_STRING(env->isolate(), "NODE_DEBUG_ENABLED")); + Local argv[] = { + FIXED_ONE_BYTE_STRING(env->isolate(), "internalMessage"), message }; - MakeCallback(env, env->process_object(), "emit", ARRAY_SIZE(args), args); -} - - -// Called from the signal watcher callback -static void EmitDebugEnabled() { - uv_async_send(&emit_debug_enabled_async); + MakeCallback(env, env->process_object(), "emit", ARRAY_SIZE(argv), argv); } -static void EnableDebug(bool wait_connect) { - // If we're called from another thread, make sure to enter the right - // v8 isolate. - node_isolate->Enter(); - - v8::Debug::SetDebugMessageDispatchHandler(DispatchMessagesDebugAgentCallback, - false); - - // Start the debug thread and it's associated TCP server on port 5858. - bool r = v8::Debug::EnableAgent("node " NODE_VERSION, - debug_port, - wait_connect); - - // Crappy check that everything went well. FIXME - assert(r); - - // Print out some information. - fprintf(stderr, "debugger listening on port %d\n", debug_port); - fflush(stderr); - - debugger_running = true; - - if (Environment::GetCurrentChecked(node_isolate) != NULL) { - EmitDebugEnabled(); +// Called from the main thread. +static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle, int status) { + if (debugger_running == false) { + fprintf(stderr, "Starting debugger agent.\n"); + EnableDebug(false); } - - node_isolate->Exit(); + Isolate::Scope isolate_scope(node_isolate); + v8::Debug::ProcessDebugMessages(); } #ifdef __POSIX__ -static void EnableDebugSignalHandler(uv_signal_t* handle, int) { - // Break once process will return execution to v8 - v8::Debug::DebugBreak(node_isolate); - - if (!debugger_running) { - fprintf(stderr, "Hit SIGUSR1 - starting debugger agent.\n"); - EnableDebug(false); - } +static void EnableDebugSignalHandler(int signo) { + // Call only async signal-safe functions here! + v8::Debug::DebugBreak(*static_cast(&node_isolate)); + uv_async_send(&dispatch_debug_messages_async); } static void RegisterSignalHandler(int signal, void (*handler)(int signal)) { struct sigaction sa; - memset(&sa, 0, sizeof(sa)); sa.sa_handler = handler; sigfillset(&sa.sa_mask); @@ -2925,22 +2906,20 @@ void DebugProcess(const FunctionCallbackInfo& args) { return ThrowErrnoException(errno, "kill"); } } + + +static int RegisterDebugSignalHandler() { + // FIXME(bnoordhuis) Should be per-isolate or per-context, not global. + RegisterSignalHandler(SIGUSR1, EnableDebugSignalHandler); + return 0; +} #endif // __POSIX__ #ifdef _WIN32 DWORD WINAPI EnableDebugThreadProc(void* arg) { - // Break once process will return execution to v8 - if (!debugger_running) { - for (int i = 0; i < 1; i++) { - fprintf(stderr, "Starting debugger agent.\r\n"); - fflush(stderr); - EnableDebug(false); - } - } - - v8::Debug::DebugBreak(node_isolate); - + v8::Debug::DebugBreak(*static_cast(&node_isolate)); + uv_async_send(&dispatch_debug_messages_async); return 0; } @@ -3105,13 +3084,6 @@ void Init(int* argc, DispatchDebugMessagesAsyncCallback); uv_unref(reinterpret_cast(&dispatch_debug_messages_async)); - // init async NODE_DEBUG_ENABLED emitter - // FIXME(bnoordhuis) Should be per-isolate or per-context, not global. - uv_async_init(uv_default_loop(), - &emit_debug_enabled_async, - EmitDebugEnabledAsyncCallback); - uv_unref(reinterpret_cast(&emit_debug_enabled_async)); - // Parse a few arguments which are specific to Node. int v8_argc; const char** v8_argv; @@ -3192,15 +3164,7 @@ void Init(int* argc, if (use_debug_agent) { EnableDebug(debug_wait_connect); } else { -#ifdef _WIN32 RegisterDebugSignalHandler(); -#else // Posix - // FIXME(bnoordhuis) Should be per-isolate or per-context, not global. - static uv_signal_t signal_watcher; - uv_signal_init(uv_default_loop(), &signal_watcher); - uv_signal_start(&signal_watcher, EnableDebugSignalHandler, SIGUSR1); - uv_unref(reinterpret_cast(&signal_watcher)); -#endif // __POSIX__ } } diff --git a/test/simple/test-debug-cluster.js b/test/simple/test-debug-cluster.js index 2364181..18e8ffa 100644 --- a/test/simple/test-debug-cluster.js +++ b/test/simple/test-debug-cluster.js @@ -42,9 +42,9 @@ child.stderr.on('data', function(data) { var assertOutputLines = common.mustCall(function() { var expectedLines = [ - 'debugger listening on port ' + 5858, - 'debugger listening on port ' + 5859, - 'debugger listening on port ' + 5860, + 'Debugger listening on port ' + 5858, + 'Debugger listening on port ' + 5859, + 'Debugger listening on port ' + 5860, ]; // Do not assume any particular order of output messages, diff --git a/test/simple/test-debug-port-cluster.js b/test/simple/test-debug-port-cluster.js index 729eb2e..7e71dff 100644 --- a/test/simple/test-debug-port-cluster.js +++ b/test/simple/test-debug-port-cluster.js @@ -48,9 +48,9 @@ child.stderr.on('data', function(data) { var assertOutputLines = common.mustCall(function() { var expectedLines = [ - 'debugger listening on port ' + port, - 'debugger listening on port ' + (port+1), - 'debugger listening on port ' + (port+2), + 'Debugger listening on port ' + port, + 'Debugger listening on port ' + (port+1), + 'Debugger listening on port ' + (port+2), ]; // Do not assume any particular order of output messages, diff --git a/test/simple/test-debug-port-from-cmdline.js b/test/simple/test-debug-port-from-cmdline.js index f98df4e..db0f1b3 100644 --- a/test/simple/test-debug-port-from-cmdline.js +++ b/test/simple/test-debug-port-from-cmdline.js @@ -51,20 +51,16 @@ function processStderrLine(line) { console.log('> ' + line); outputLines.push(line); - if (/debugger listening/.test(line)) { + if (/Debugger listening/.test(line)) { assertOutputLines(); process.exit(); } } function assertOutputLines() { - var startLog = process.platform === 'win32' - ? 'Starting debugger agent.' - : 'Hit SIGUSR1 - starting debugger agent.'; - var expectedLines = [ - startLog, - 'debugger listening on port ' + debugPort + 'Starting debugger agent.', + 'Debugger listening on port ' + debugPort ]; assert.equal(outputLines.length, expectedLines.length); diff --git a/test/simple/test-debug-signal-cluster.js b/test/simple/test-debug-signal-cluster.js index c2051e6..7b2b78c 100644 --- a/test/simple/test-debug-signal-cluster.js +++ b/test/simple/test-debug-signal-cluster.js @@ -61,17 +61,13 @@ process.on('exit', function onExit() { }); function assertOutputLines() { - var startLog = process.platform === 'win32' - ? 'Starting debugger agent.' - : 'Hit SIGUSR1 - starting debugger agent.'; - var expectedLines = [ - startLog, - 'debugger listening on port ' + 5858, - startLog, - 'debugger listening on port ' + 5859, - startLog, - 'debugger listening on port ' + 5860, + 'Starting debugger agent.', + 'Debugger listening on port ' + 5858, + 'Starting debugger agent.', + 'Debugger listening on port ' + 5859, + 'Starting debugger agent.', + 'Debugger listening on port ' + 5860, ]; // Do not assume any particular order of output messages, diff --git a/test/simple/test-debugger-client.js b/test/simple/test-debugger-client.js index fd18ec1..25bb409 100644 --- a/test/simple/test-debugger-client.js +++ b/test/simple/test-debugger-client.js @@ -182,7 +182,7 @@ function doTest(cb, done) { nodeProcess.stderr.resume(); b += data; if (didTryConnect == false && - b.match(/debugger listening on port/)) { + b.match(/Debugger listening on port/)) { didTryConnect = true; setTimeout(function() { @@ -224,4 +224,3 @@ process.on('exit', function(code) { if (!code) assert.equal(expectedConnections, connectCount); }); -