debugger: fix SIGUSR1 bootstrap race condition

Before this commit, the SIGUSR1 signal handler wasn't installed until
late in the bootstrapping process and we were prone to miss signals
sent by other processes.

This commit installs an early-boot signal handler that merely records
the fact that we received a signal.  Once the debugger infrastructure
is in place, the signal is re-raised, kickstarting the debugger.

Among other things, this means that simple/test-debugger-client is
now _much_ less likely to fail.

diff --git a/src/node.cc b/src/node.cc
index 2c874ce..1a3995c 100644 (file)
--- a/src/node.cc
+++ b/src/node.cc
@@ -2874,6 +2874,22 @@ static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle, int status) {
 
 
 #ifdef __POSIX__
+static volatile sig_atomic_t caught_early_debug_signal;
+
+
+static void EarlyDebugSignalHandler(int signo) {
+  caught_early_debug_signal = 1;
+}
+
+
+static void InstallEarlyDebugSignalHandler() {
+  struct sigaction sa;
+  memset(&sa, 0, sizeof(sa));
+  sa.sa_handler = EarlyDebugSignalHandler;
+  sigaction(SIGUSR1, &sa, NULL);
+}
+
+
 static void EnableDebugSignalHandler(int signo) {
   // Call only async signal-safe functions here!
   v8::Debug::DebugBreak(*static_cast<Isolate* volatile*>(&node_isolate));
@@ -2911,6 +2927,9 @@ void DebugProcess(const FunctionCallbackInfo<Value>& args) {
 static int RegisterDebugSignalHandler() {
   // FIXME(bnoordhuis) Should be per-isolate or per-context, not global.
   RegisterSignalHandler(SIGUSR1, EnableDebugSignalHandler);
+  // If we caught a SIGUSR1 during the bootstrap process, re-raise it
+  // now that the debugger infrastructure is in place.
+  if (caught_early_debug_signal) raise(SIGUSR1);
   return 0;
 }
 #endif  // __POSIX__
@@ -3270,6 +3289,11 @@ Environment* CreateEnvironment(Isolate* isolate,
 
 
 int Start(int argc, char** argv) {
+#if !defined(_WIN32)
+  // Try hard not to lose SIGUSR1 signals during the bootstrap process.
+  InstallEarlyDebugSignalHandler();
+#endif
+
   assert(argc > 0);
 
   // Hack around with the argv pointer. Used for process.title = "blah".

diff --git a/test/simple/test-debugger-client.js b/test/simple/test-debugger-client.js
index 25bb409..21ddf5e 100644 (file)
--- a/test/simple/test-debugger-client.js
+++ b/test/simple/test-debugger-client.js
@@ -181,11 +181,15 @@ function doTest(cb, done) {
     console.error('got stderr data %j', data);
     nodeProcess.stderr.resume();
     b += data;
-    if (didTryConnect == false &&
-        b.match(/Debugger listening on port/)) {
+    if (didTryConnect === false && b.match(/Debugger listening on port/)) {
       didTryConnect = true;
 
-      setTimeout(function() {
+      // The timeout is here to expose a race in the bootstrap process.
+      // Without the early SIGUSR1 debug handler, it effectively results
+      // in an infinite ECONNREFUSED loop.
+      setTimeout(tryConnect, 100);
+
+      function tryConnect() {
         // Wait for some data before trying to connect
         var c = new debug.Client();
         console.error('>>> connecting...');
@@ -202,7 +206,11 @@ function doTest(cb, done) {
             done();
           });
         });
-      });
+        c.on('error', function(err) {
+          if (err.code !== 'ECONNREFUSED') throw err;
+          setTimeout(tryConnect, 10);
+        });
+      }
     }
   });
 }