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 (file)
--- 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<Object> message = Object::New();
-  message->Set(FIXED_ONE_BYTE_STRING(node_isolate, "cmd"),
-               FIXED_ONE_BYTE_STRING(node_isolate, "NODE_DEBUG_ENABLED"));
-  Local<Value> 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<Value> 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<Isolate* volatile*>(&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<Value>& 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<Isolate* volatile*>(&node_isolate));
+  uv_async_send(&dispatch_debug_messages_async);
   return 0;
 }
 
@@ -3105,13 +3084,6 @@ void Init(int* argc,
                 DispatchDebugMessagesAsyncCallback);
   uv_unref(reinterpret_cast<uv_handle_t*>(&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<uv_handle_t*>(&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<uv_handle_t*>(&signal_watcher));
-#endif  // __POSIX__
   }
 }
 

diff --git a/test/simple/test-debug-cluster.js b/test/simple/test-debug-cluster.js
index 2364181..18e8ffa 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);
 });
-