From af3fe5624bf29b0cf92a86dd866e346204d956a2 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Wed, 24 Sep 2014 14:21:31 -0700 Subject: [PATCH 1/3] Replace keepalives with a check if the parent pid is still running. Downsides: * 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's causing #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. Untested so far because I haven't been able to actually kill a parent process in such a way that the child stays alive. --- packages/webapp/webapp_server.js | 40 +++++++++++++++++++++++++++++--- tools/run-app.js | 19 ++------------- 2 files changed, 39 insertions(+), 20 deletions(-) diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index bad2db3532..80582710c2 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -35,7 +35,11 @@ var bundledJsCssPrefix; // 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; @@ -54,6 +58,21 @@ var initKeepalive = function () { }, 3000); }; +// As a replacement to the old keepalives mechanism, check for a running +// parent every few seconds. Exit if the parent is not running. +var startCheckForLiveParent = function (parentPid) { + if (parentPid) { + setInterval(function () { + try { + process.kill(parentPid, 0); + } catch (err) { + console.log("Parent process is dead! Exiting."); + process.exit(1); + } + }); + } +}; + var sha1 = function (contents) { var hash = crypto.createHash('sha1'); @@ -747,7 +766,18 @@ var runWebAppServer = function () { // 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. @@ -755,7 +785,7 @@ var runWebAppServer = function () { var host = process.env.BIND_IP; var localIp = host || '0.0.0.0'; httpServer.listen(localPort, localIp, Meteor.bindEnvironment(function() { - if (expectKeepalives) + if (expectKeepalives || parentPid) console.log("LISTENING"); // must match run-app.js var proxyBinding; @@ -810,8 +840,12 @@ var runWebAppServer = function () { console.error(e && e.stack); })); - if (expectKeepalives) + if (expectKeepalives) { initKeepalive(); + } + if (parentPid) { + startCheckForLiveParent(parentPid); + } return 'DAEMON'; }; }; diff --git a/tools/run-app.js b/tools/run-app.js index 0d926f3a49..64d1f167b6 100644 --- a/tools/run-app.js +++ b/tools/run-app.js @@ -65,7 +65,6 @@ var AppProcess = function (options) { self.settings = options.settings; self.proc = null; - self.keepaliveTimer = null; self.madeExitCallback = false; }; @@ -123,17 +122,6 @@ _.extend(AppProcess.prototype, { // exception and the whole app dies. // http://stackoverflow.com/questions/2893458/uncatchable-errors-in-node-js self.proc.stdin.on('error', function () {}); - - // Keepalive so child process can detect when we die - self.keepaliveTimer = setInterval(function () { - try { - if (self.proc && self.proc.pid && - self.proc.stdin && self.proc.stdin.write) - self.proc.stdin.write('k'); - } catch (e) { - // do nothing. this fails when the process dies. - } - }, 2000); }, _maybeCallOnExit: function (code, signal) { @@ -156,10 +144,6 @@ _.extend(AppProcess.prototype, { } self.proc = null; - if (self.keepaliveTimer) - clearInterval(self.keepaliveTimer); - self.keepaliveTimer = null; - self.onListen = null; self.onExit = null; }, @@ -207,7 +191,8 @@ _.extend(AppProcess.prototype, { // Old-style bundle var opts = _.clone(self.nodeOptions); opts.push(path.join(self.bundlePath, 'main.js')); - opts.push('--keepalive'); + opts.push('--parent-pid'); + opts.push(process.pid); return child_process.spawn(process.execPath, opts, { env: self._computeEnvironment() From 4b739b0684e24fe0efd4fc037e4ddab69324af8e Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Thu, 25 Sep 2014 10:29:13 -0700 Subject: [PATCH 2/3] Add test for runner process being SIGKILLed. --- .../apps/app-prints-pid/.meteor/.gitignore | 1 + .../apps/app-prints-pid/.meteor/packages | 6 +++ .../tests/apps/app-prints-pid/.meteor/release | 1 + tools/tests/apps/app-prints-pid/print.js | 5 +++ tools/tests/run.js | 38 +++++++++++++++++++ 5 files changed, 51 insertions(+) create mode 100644 tools/tests/apps/app-prints-pid/.meteor/.gitignore create mode 100644 tools/tests/apps/app-prints-pid/.meteor/packages create mode 100644 tools/tests/apps/app-prints-pid/.meteor/release create mode 100644 tools/tests/apps/app-prints-pid/print.js diff --git a/tools/tests/apps/app-prints-pid/.meteor/.gitignore b/tools/tests/apps/app-prints-pid/.meteor/.gitignore new file mode 100644 index 0000000000..4083037423 --- /dev/null +++ b/tools/tests/apps/app-prints-pid/.meteor/.gitignore @@ -0,0 +1 @@ +local diff --git a/tools/tests/apps/app-prints-pid/.meteor/packages b/tools/tests/apps/app-prints-pid/.meteor/packages new file mode 100644 index 0000000000..aba458ef9a --- /dev/null +++ b/tools/tests/apps/app-prints-pid/.meteor/packages @@ -0,0 +1,6 @@ +# Meteor packages used by this project, one per line. +# +# 'meteor add' and 'meteor remove' will edit this file for you, +# but you can also edit it by hand. + +meteor-platform diff --git a/tools/tests/apps/app-prints-pid/.meteor/release b/tools/tests/apps/app-prints-pid/.meteor/release new file mode 100644 index 0000000000..621e94f0ec --- /dev/null +++ b/tools/tests/apps/app-prints-pid/.meteor/release @@ -0,0 +1 @@ +none diff --git a/tools/tests/apps/app-prints-pid/print.js b/tools/tests/apps/app-prints-pid/print.js new file mode 100644 index 0000000000..6a393c5a1b --- /dev/null +++ b/tools/tests/apps/app-prints-pid/print.js @@ -0,0 +1,5 @@ +if (Meteor.isServer) { + Meteor.startup(function () { + console.log("My pid is " + process.pid); + }); +} diff --git a/tools/tests/run.js b/tools/tests/run.js index adf08a3561..ff0344edc6 100644 --- a/tools/tests/run.js +++ b/tools/tests/run.js @@ -340,3 +340,41 @@ selftest.define("run with mongo crash", ["checkout"], function () { run.expectEnd(); run.expectExit(254); }); + +// Test that when the parent runner process is SIGKILLed, the child +// process exits also. +selftest.define("run and SIGKILL parent process", function () { + var s = new Sandbox(); + var run; + + s.createApp("myapp", "app-prints-pid"); + s.cd("myapp"); + + run = s.run(); + run.waitSecs(10); + var match = run.match(/My pid is (\d+)/); + var childPid; + if (! match || ! match[1]) { + selftest.fail("No pid printed"); + } + childPid = match[1]; + + process.kill(run.proc.pid, "SIGKILL"); + // This sleep should match the interval at which the child checks if + // the parent is still alive, in packages/webapp/webapp_server.js. + utils.sleepMs(3000); + + // Send the child process a signal of 0. If there is no error, it + // means that the process is still running, which is not what we + // expect. + var caughtError; + try { + process.kill(childPid, 0); + } catch (err) { + caughtError = err; + } + + if (! caughtError) { + selftest.fail("Child process " + childPid + " is still running"); + } +}); From 2f47a81395322de022af9680470d3ef4a3e74133 Mon Sep 17 00:00:00 2001 From: Emily Stark Date: Thu, 25 Sep 2014 15:21:45 -0700 Subject: [PATCH 3/3] Comments from nim, ben --- tools/run-app.js | 3 +-- tools/tests/run.js | 7 ++++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/run-app.js b/tools/run-app.js index 64d1f167b6..075a135d5b 100644 --- a/tools/run-app.js +++ b/tools/run-app.js @@ -191,8 +191,7 @@ _.extend(AppProcess.prototype, { // Old-style bundle var opts = _.clone(self.nodeOptions); opts.push(path.join(self.bundlePath, 'main.js')); - opts.push('--parent-pid'); - opts.push(process.pid); + opts.push('--parent-pid', process.pid); return child_process.spawn(process.execPath, opts, { env: self._computeEnvironment() diff --git a/tools/tests/run.js b/tools/tests/run.js index ff0344edc6..74f1b890a9 100644 --- a/tools/tests/run.js +++ b/tools/tests/run.js @@ -360,9 +360,10 @@ selftest.define("run and SIGKILL parent process", function () { childPid = match[1]; process.kill(run.proc.pid, "SIGKILL"); - // This sleep should match the interval at which the child checks if - // the parent is still alive, in packages/webapp/webapp_server.js. - utils.sleepMs(3000); + // This sleep should be a little more time than the interval at which + // the child checks if the parent is still alive, in + // packages/webapp/webapp_server.js. + utils.sleepMs(3500); // Send the child process a signal of 0. If there is no error, it // means that the process is still running, which is not what we