Detect inspector client attachment by timing debugger keyword. (#9172)

The `meteor debug` command behaves like Node's `--inspect-brk` flag, in
that it attempts to pause the server before executing any server code.

However, simply passing the `--inspect-brk` flag to Node causes execution
to pause on the very first line of code, which is not good for setting any
breakpoints, because no server code has actually loaded yet.

Instead, the `meteor debug` command uses Node's `--inspect` flag to enable
debugging without an initial pause, then manually pauses at an appropriate
moment during server startup. Ideally, the pause should last until an
inspector client has been attached to the process, at which point the
developer has a chance to set any desired breakpoints, then clicks the
continue button to proceed with server startup.

The most difficult part of this process is detecting when the inspector
client has attached. Previously, the parent process listened for the child
process to print a "Debugger attached" message to STDERR, which happens as
a result of this `fprintf` call in Native C++:
7cff6e80bf/src/inspector_io.cc (L396)

However, this message was not printed in some cases, especially on Windows
(#9165), and required inter-process communication even in the ideal case.
All of that logic is gone now, thanks to this commit.

This commit takes advantage of a difference in behavior of the `debugger`
keyword depending on whether or not an inspector client is attached. When
no client is attached, the `debugger` keyword is a no-op that takes no
time (or very little time) to execute. Once a client has attached, the
`debugger` keyword triggers a breakpoint that lasts until the developer
explicitly continues execution through the client UI. Needless to say,
this makes the `debugger` keyword take longer than a no-op.

Because the `debugger` keyword does nothing until a client connects, we
can safely poll a `pause` function containing a `debugger` keyword at a
frequent interval (say, every 500ms). Once a client connects, the
`debugger` keyword will become active, pausing the server at exactly the
point we hoped. The difference is easy to detect by timing the `pause()`
function call. Once the `debugger` keyword becomes active, we stop polling
and allow server startup to continue.

Elegant!

Fixes #9165.

tools/runners/run-app.js

@@ -99,13 +99,6 @@ _.extend(AppProcess.prototype, {
     });
 
     eachline(self.proc.stderr, function (line) {
-      if (self.debugPort &&
-          line.indexOf("Debugger attached") >= 0) {
-        self.proc.send({
-          meteorDebugCommand: "continue"
-        });
-      }
-
       runLog.logAppOutput(line, true);
     });
 

tools/static-assets/server/boot.js

@@ -45,34 +45,60 @@ var parsedSourceMaps = {};
 const meteorDebugFuture =
   process.env.METEOR_INSPECT_BRK ? new Future : null;
 
-if (meteorDebugFuture) {
-  process.on("message", function onDebugMessage(msg) {
-    if (msg && msg.meteorDebugCommand === "continue") {
-      process.removeListener("message", onDebugMessage);
-      // After the "continue" message is received, the Chrome DevTools
-      // debugger still needs a small amount of time to get ready to pause
-      // at the debugger statement in ./debug.js.
-      setTimeout(() => meteorDebugFuture.return(true), 500);
-    }
-  });
-}
-
 function maybeWaitForDebuggerToAttach() {
   if (meteorDebugFuture) {
-    // This setTimeout not only puts a reasonable time limit on the
-    // debugger attaching, but also keeps the process alive by preventing
-    // the event loop from running empty while the Fiber yields.
-    const timer = setTimeout(() => {
-      console.error("Debugger did not attach after 5 minutes; continuing.");
-      meteorDebugFuture.return(false);
-    }, 5 * 60 * 1000);
+    const { pause } = require("./debug.js");
+    const pauseThresholdMs = 50;
+    const pollIntervalMs = 500;
+    const waitStartTimeMs = +new Date;
+    const waitLimitMinutes = 5;
+    const waitLimitMs = waitLimitMinutes * 60 * 1000;
 
-    const shouldPause = meteorDebugFuture.wait();
-    clearTimeout(timer);
+    // This setTimeout not only waits for the debugger to attach, but also
+    // keeps the process alive by preventing the event loop from running
+    // empty while the main Fiber yields.
+    setTimeout(function poll() {
+      const pauseStartTimeMs = +new Date;
 
-    if (shouldPause) {
-      require("./debug.js");
-    }
+      if (pauseStartTimeMs - waitStartTimeMs > waitLimitMs) {
+        console.error(
+          `Debugger did not attach after ${waitLimitMinutes} minutes; continuing.`
+        );
+
+        meteorDebugFuture.return();
+
+      } else {
+        // This pause function contains a debugger keyword that will only
+        // act as a breakpoint once a debugging client has attached to the
+        // process, so we keep calling pause() until the first time it
+        // takes at least pauseThresholdMs, which indicates that a client
+        // must be attached. The only other signal of a client attaching
+        // is an unreliable "Debugger attached" message printed to stderr
+        // by native C++ code, which requires the parent process to listen
+        // for that message and then process.send a message back to this
+        // process. By comparison, this polling strategy tells us exactly
+        // what we want to know: "Is the debugger keyword enabled yet?"
+        pause();
+
+        if (new Date - pauseStartTimeMs > pauseThresholdMs) {
+          // If the pause() function call took a meaningful amount of
+          // time, we can conclude the debugger keyword must be active,
+          // which means a debugging client must be connected, which means
+          // we should stop polling and let the main Fiber continue.
+          meteorDebugFuture.return();
+
+        } else {
+          // If the pause() function call didn't take a meaningful amount
+          // of time to execute, then the debugger keyword must not have
+          // caused a pause, which means a debugging client must not be
+          // connected, which means we should keep polling.
+          setTimeout(poll, pollIntervalMs);
+        }
+      }
+    }, pollIntervalMs);
+
+    // The polling will continue while we wait here.
+    meteorDebugFuture.wait();
   }
 }
 

tools/static-assets/server/debug.js

@@ -1,6 +1,8 @@
 
-// The debugger pauses here when you run `meteor debug`, so that you can
-// set breakpoints or add `debugger` statements to your server code before
-// the code begins executing. Once you have set any breakpoints you wish
-// to set, click the |▶ button to continue.
-debugger;
+exports.pause = function () {
+  // The debugger pauses here when you run `meteor debug`, so that you can
+  // set breakpoints or add `debugger` statements to your server code
+  // before the code begins executing. Once you have set any breakpoints
+  // you wish to set, click the |▶ button to continue.
+  debugger;
+};