From 87ebeec54b6ac16da5584df2bd96ba400cb5dc9b Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 3 Oct 2017 12:11:21 -0400 Subject: [PATCH] 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++: https://github.com/nodejs/node/blob/7cff6e80bfd2f61ed67ff07af87af3ab63860273/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 | 7 --- tools/static-assets/server/boot.js | 74 +++++++++++++++++++---------- tools/static-assets/server/debug.js | 12 +++-- 3 files changed, 57 insertions(+), 36 deletions(-) diff --git a/tools/runners/run-app.js b/tools/runners/run-app.js index 8a19eaf510..1b92c49f27 100644 --- a/tools/runners/run-app.js +++ b/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); }); diff --git a/tools/static-assets/server/boot.js b/tools/static-assets/server/boot.js index d30c5cc520..422e3e2818 100644 --- a/tools/static-assets/server/boot.js +++ b/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(); } } diff --git a/tools/static-assets/server/debug.js b/tools/static-assets/server/debug.js index 3b4f848fac..4532ae43e6 100644 --- a/tools/static-assets/server/debug.js +++ b/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; \ No newline at end of file +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; +}; \ No newline at end of file