Refactor parent pid check; drop --keepalive

This commit moves parent pid process from the webapp package to the boot
script. This means that daemonized apps without webapp will also exit
when the runner exits, if run from the runner. (For example, several
self-tests such as 'autoupdate' no longer leak node processes.) This is
controlled via the $METEOR_PARENT_PID environment variable instead of
from command line arguments, in order to make fewer assumptions about
how Meteor apps process arguments.

This also drops the old --keepalive support (which already has stopped
being used by the dev mode runner or any MDG deployment platforms).
Neither --parent-pid nor --keepalive were documented beforehand, and
--keepalive was already deprecated before 1.0.

These flags used to also incidentally trigger printing the LISTENING
line; this is now controlled by $METEOR_PRINT_ON_LISTEN.

Fixes #3315.

packages/webapp/webapp_server.js

@@ -30,68 +30,6 @@ var archPath = {};
 
 var bundledJsCssPrefix;
 
-// Keepalives so that when the outer server dies unceremoniously and
-// doesn't kill us, we quit ourselves. A little gross, but better than
-// pidfiles.
-// XXX This should really be part of the boot script, not the webapp package.
-//     Or we should just get rid of it, and rely on containerization.
-//
-// XXX COMPAT WITH 0.9.2.2
-// Keepalives have been replaced with a check that the parent pid is
-// still running. We keep the --keep-alive option for backwards
-// compatibility.
-var initKeepalive = function () {
-  var keepaliveCount = 0;
-
-  process.stdin.on('data', function (data) {
-    keepaliveCount = 0;
-  });
-
-  process.stdin.resume();
-
-  setInterval(function () {
-    keepaliveCount ++;
-    if (keepaliveCount >= 3) {
-      console.log("Failed to receive keepalive! Exiting.");
-      process.exit(1);
-    }
-  }, 3000);
-};
-
-// Check that we have a pid that looks like an integer (non-decimal
-// integer is okay).
-var validPid = function (pid) {
-  return ! isNaN(+pid);
-};
-
-// As a replacement to the old keepalives mechanism, check for a running
-// parent every few seconds. Exit if the parent is not running.
-//
-// Two caveats to this strategy:
-// * Doesn't catch the case where the parent is CPU-hogging (but maybe we
-//   don't want to catch that case anyway, since the bundler not yielding
-//   is what caused #2536).
-// * Could be fooled by pid re-use, i.e. if another process comes up and
-//   takes the parent process's place before the child process dies.
-var startCheckForLiveParent = function (parentPid) {
-  if (parentPid) {
-    if (! validPid(parentPid)) {
-      console.error("--parent-pid must be a valid process ID.");
-      process.exit(1);
-    }
-
-    setInterval(function () {
-      try {
-        process.kill(parentPid, 0);
-      } catch (err) {
-        console.error("Parent process is dead! Exiting.");
-        process.exit(1);
-      }
-    }, 3000);
-  }
-};
-
-
 var sha1 = function (contents) {
   var hash = crypto.createHash('sha1');
   hash.update(contents);
@@ -812,29 +750,15 @@ var runWebAppServer = function () {
     // main happens post startup hooks, so we don't need a Meteor.startup() to
     // ensure this happens after the galaxy package is loaded.
     var AppConfig = Package["application-configuration"].AppConfig;
-    // We used to use the optimist npm package to parse argv here, but it's
-    // overkill (and no longer in the dev bundle). Just assume any instance of
-    // '--keepalive' is a use of the option.
-    // XXX COMPAT WITH 0.9.2.2
-    // We used to expect keepalives to be written to stdin every few
-    // seconds; now we just check if the parent process is still alive
-    // every few seconds.
-    var expectKeepalives = _.contains(argv, '--keepalive');
-    // XXX Saddest argument parsing ever, should we add optimist back to
-    // the dev bundle?
-    var parentPid = null;
-    var parentPidIndex = _.indexOf(argv, "--parent-pid");
-    if (parentPidIndex !== -1) {
-      parentPid = argv[parentPidIndex + 1];
-    }
+
     WebAppInternals.generateBoilerplate();
 
     // only start listening after all the startup code has run.
     var localPort = parseInt(process.env.PORT) || 0;
     var host = process.env.BIND_IP;
     var localIp = host || '0.0.0.0';
     httpServer.listen(localPort, localIp, Meteor.bindEnvironment(function() {
-      if (expectKeepalives || parentPid)
+      if (process.env.METEOR_PRINT_ON_LISTEN)
         console.log("LISTENING"); // must match run-app.js
       var proxyBinding;
 
@@ -889,12 +813,6 @@ var runWebAppServer = function () {
       console.error(e && e.stack);
     }));
 
-    if (expectKeepalives) {
-      initKeepalive();
-    }
-    if (parentPid) {
-      startCheckForLiveParent(parentPid);
-    }
     return 'DAEMON';
   };
 };
@@ -1214,4 +1132,3 @@ WebAppInternals.addStaticJs = function (contents) {
 // Exported for tests
 WebAppInternals.getBoilerplate = getBoilerplate;
 WebAppInternals.additionalStaticJs = additionalStaticJs;
-WebAppInternals.validPid = validPid;

packages/webapp/webapp_tests.js

@@ -134,17 +134,6 @@ Tinytest.add("webapp - additional static javascript", function (test) {
   WebAppInternals.setInlineScriptsAllowed(origInlineScriptsAllowed);
 });
 
-Tinytest.add("webapp - valid pid check", function (test) {
-  test.isTrue(WebAppInternals.validPid(123));
-  test.isTrue(WebAppInternals.validPid("123"));
-  test.isTrue(WebAppInternals.validPid(0x123));
-  test.isTrue(WebAppInternals.validPid("0x123"));
-
-  test.isFalse(WebAppInternals.validPid("foo123"));
-  test.isFalse(WebAppInternals.validPid("foobar"));
-  test.isFalse(WebAppInternals.validPid("123foo"));
-});
-
 // Regression test: `generateBoilerplateInstance` should not change
 // `__meteor_runtime_config__`.
 Tinytest.add("webapp - generating boilerplate should not change runtime config", function (test) {

tools/bundler.js

@@ -1695,6 +1695,9 @@ _.extend(ServerTarget.prototype, {
     // Server bootstrap
     builder.write('boot.js',
                   { file: files.pathJoin(__dirname, 'server', 'boot.js') });
+    builder.write(
+      'boot-utils.js',
+      { file: files.pathJoin(__dirname, 'server', 'boot-utils.js') });
     builder.write('shell.js',
                   { file: files.pathJoin(__dirname, 'server', 'shell.js') });
 

tools/run-app.js

@@ -84,8 +84,9 @@ _.extend(AppProcess.prototype, {
     var eachline = require('eachline');
     eachline(self.proc.stdout, 'utf8', fiberHelpers.inBareFiber(function (line) {
       if (line.match(/^LISTENING\s*$/)) {
-        // This is the child process telling us that it's ready to
-        // receive connections.
+        // This is the child process telling us that it's ready to receive
+        // connections.  (It does this because we told it to with
+        // $METEOR_PRINT_ON_LISTEN.)
         self.onListen && self.onListen();
 
       } else {
@@ -193,6 +194,11 @@ _.extend(AppProcess.prototype, {
 
     env.ENABLE_METEOR_SHELL = 'true';
 
+    env.METEOR_PARENT_PID =
+      process.env.METEOR_BAD_PARENT_PID_FOR_TEST ? "foobar" : process.pid;
+
+    env.METEOR_PRINT_ON_LISTEN = 'true';
+
     return env;
   },
 
@@ -213,10 +219,7 @@ _.extend(AppProcess.prototype, {
       opts.push("--debug-brk=" + attach.suggestedDebugBrkPort);
     }
 
-    opts.push(
-      entryPoint, '--parent-pid',
-      process.env.METEOR_BAD_PARENT_PID_FOR_TEST ? "foobar" : process.pid
-    );
+    opts.push(entryPoint);
 
     var child = child_process.spawn(process.execPath, opts, {
       env: self._computeEnvironment()

tools/selftest.js

@@ -52,6 +52,19 @@ var expectEqual = markStack(function (actual, expected) {
   }
 });
 
+// Call from a test to assert that 'actual' is truthy.
+var expectTrue = markStack(function (actual) {
+  if (! actual) {
+    throw new TestFailure('not-true');
+  }
+});
+// Call from a test to assert that 'actual' is falsey.
+var expectFalse = markStack(function (actual) {
+  if (actual) {
+    throw new TestFailure('not-false');
+  }
+});
+
 var expectThrows = markStack(function (f) {
   var threw = false;
   try {
@@ -1823,6 +1836,8 @@ _.extend(exports, {
   fail: fail,
   expectEqual: expectEqual,
   expectThrows: expectThrows,
+  expectTrue: expectTrue,
+  expectFalse: expectFalse,
   execFileSync: execFileSync,
   doOrThrow: doOrThrow,
   testPackageServerUrl: config.getTestPackageServerUrl()

tools/server/boot-utils.js

@@ -0,0 +1,7 @@
+// Separated from boot.js for testing.
+
+// Check that we have a pid that looks like an integer (non-decimal
+// integer is okay).
+exports.validPid = function (pid) {
+  return ! isNaN(+pid);
+};

tools/server/boot.js

@@ -5,6 +5,8 @@ var Future = require("fibers/future");
 var _ = require('underscore');
 var sourcemap_support = require('source-map-support');
 
+var bootUtils = require('./boot-utils.js');
+
 // This code is duplicated in tools/main.js.
 var MIN_NODE_VERSION = 'v0.10.33';
 
@@ -82,6 +84,34 @@ if (process.env.ENABLE_METEOR_SHELL) {
   require('./shell.js').listen();
 }
 
+// As a replacement to the old keepalives mechanism, check for a running
+// parent every few seconds. Exit if the parent is not running.
+//
+// Two caveats to this strategy:
+// * Doesn't catch the case where the parent is CPU-hogging (but maybe we
+//   don't want to catch that case anyway, since the bundler not yielding
+//   is what caused #2536).
+// * Could be fooled by pid re-use, i.e. if another process comes up and
+//   takes the parent process's place before the child process dies.
+var startCheckForLiveParent = function (parentPid) {
+  if (parentPid) {
+    if (! bootUtils.validPid(parentPid)) {
+      console.error("METEOR_PARENT_PID must be a valid process ID.");
+      process.exit(1);
+    }
+
+    setInterval(function () {
+      try {
+        process.kill(parentPid, 0);
+      } catch (err) {
+        console.error("Parent process is dead! Exiting.");
+        process.exit(1);
+      }
+    }, 3000);
+  }
+};
+
+
 Fiber(function () {
   _.each(serverJson.load, function (fileInfo) {
     var code = fs.readFileSync(path.resolve(serverDir, fileInfo.path));
@@ -209,4 +239,8 @@ Fiber(function () {
   // XXX hack, needs a better way to keep alive
   if (exitCode !== 'DAEMON')
     process.exit(exitCode);
+
+  if (process.env.METEOR_PARENT_PID) {
+    startCheckForLiveParent(process.env.METEOR_PARENT_PID);
+  }
 }).run();

tools/tests/boot-tests.js

@@ -0,0 +1,13 @@
+var selftest = require('../selftest.js');
+
+selftest.define("boot utils", function (options) {
+  var bootUtils = require('../server/boot-utils.js');
+  selftest.expectTrue(bootUtils.validPid(123));
+  selftest.expectTrue(bootUtils.validPid("123"));
+  selftest.expectTrue(bootUtils.validPid(0x123));
+  selftest.expectTrue(bootUtils.validPid("0x123"));
+
+  selftest.expectFalse(bootUtils.validPid("foo123"));
+  selftest.expectFalse(bootUtils.validPid("foobar"));
+  selftest.expectFalse(bootUtils.validPid("123foo"));
+});

tools/tests/run.js

@@ -363,7 +363,7 @@ selftest.define("run and SIGKILL parent process", function () {
 
   run.stop();
 
-  // Test that passing a bad pid in --parent-pid logs an error and exits
+  // Test that passing a bad pid in $METEOR_PARENT_PID logs an error and exits
   // immediately.
   s.set("METEOR_BAD_PARENT_PID_FOR_TEST", "t");
   run = s.run();