From eed4d8c2119fc75aea95b7b45bb2cbefa0e5277a Mon Sep 17 00:00:00 2001 From: SteveSandersonMS Date: Fri, 19 Aug 2016 16:38:39 -0700 Subject: [PATCH] Child Node processes poll and exit when parent has exited. Fixes #270 --- .../Content/Node/entrypoint-http.js | 69 ++++++++++++ .../Content/Node/entrypoint-socket.js | 103 +++++++++++++++--- .../HostingModels/OutOfProcessNodeInstance.cs | 3 +- .../TypeScript/HttpNodeInstanceEntryPoint.ts | 3 + .../SocketNodeInstanceEntryPoint.ts | 3 + .../TypeScript/Util/ExitWhenParentExits.ts | 62 +++++++++++ 6 files changed, 225 insertions(+), 18 deletions(-) create mode 100644 src/Microsoft.AspNetCore.NodeServices/TypeScript/Util/ExitWhenParentExits.ts diff --git a/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-http.js b/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-http.js index fef873e..76a00ef 100644 --- a/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-http.js +++ b/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-http.js @@ -58,6 +58,7 @@ var http = __webpack_require__(3); var path = __webpack_require__(4); var ArgsUtil_1 = __webpack_require__(5); + var ExitWhenParentExits_1 = __webpack_require__(6); // Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct // reference to Node's runtime 'require' function. var dynamicRequire = eval('require'); @@ -121,6 +122,7 @@ // Signal to the NodeServices base class that we're ready to accept invocations console.log('[Microsoft.AspNetCore.NodeServices:Listening]'); }); + ExitWhenParentExits_1.exitWhenParentExits(parseInt(parsedArgs.parentPid)); function readRequestBodyAsJson(request, callback) { var requestBodyAsString = ''; request @@ -208,5 +210,72 @@ exports.parseArgs = parseArgs; +/***/ }, +/* 6 */ +/***/ function(module, exports) { + + /* + In general, we want the Node child processes to be terminated as soon as the parent .NET processes exit, + because we have no further use for them. If the .NET process shuts down gracefully, it will run its + finalizers, one of which (in OutOfProcessNodeInstance.cs) will kill its associated Node process immediately. + + But if the .NET process is terminated forcefully (e.g., on Linux/OSX with 'kill -9'), then it won't have + any opportunity to shut down its child processes, and by default they will keep running. In this case, it's + up to the child process to detect this has happened and terminate itself. + + There are many possible approaches to detecting when a parent process has exited, most of which behave + differently between Windows and Linux/OS X: + + - On Windows, the parent process can mark its child as being a 'job' that should auto-terminate when + the parent does (http://stackoverflow.com/a/4657392). Not cross-platform. + - The child Node process can get a callback when the parent disconnects (process.on('disconnect', ...)). + But despite http://stackoverflow.com/a/16487966, no callback fires in any case I've tested (Windows / OS X). + - The child Node process can get a callback when its stdin/stdout are disconnected, as described at + http://stackoverflow.com/a/15693934. This works well on OS X, but calling stdout.resume() on Windows + causes the process to terminate prematurely. + - I don't know why, but on Windows, it's enough to invoke process.stdin.resume(). For some reason this causes + the child Node process to exit as soon as the parent one does, but I don't see this documented anywhere. + - You can poll to see if the parent process, or your stdin/stdout connection to it, is gone + - You can directly pass a parent process PID to the child, and then have the child poll to see if it's + still running (e.g., using process.kill(pid, 0), which doesn't kill it but just tests whether it exists, + as per https://nodejs.org/api/process.html#process_process_kill_pid_signal) + - Or, on each poll, you can try writing to process.stdout. If the parent has died, then this will throw. + However I don't see this documented anywhere. It would be nice if you could just poll for whether or not + process.stdout is still connected (without actually writing to it) but I haven't found any property whose + value changes until you actually try to write to it. + + Of these, the only cross-platform approach that is actually documented as a valid strategy is simply polling + to check whether the parent PID is still running. So that's what we do here. + */ + "use strict"; + var pollIntervalMs = 1000; + function exitWhenParentExits(parentPid) { + setInterval(function () { + if (!processExists(parentPid)) { + // Can't log anything at this point, because out stdout was connected to the parent, + // but the parent is gone. + process.exit(); + } + }, pollIntervalMs); + } + exports.exitWhenParentExits = exitWhenParentExits; + function processExists(pid) { + try { + // Sending signal 0 - on all platforms - tests whether the process exists. As long as it doesn't + // throw, that means it does exist. + process.kill(pid, 0); + return true; + } + catch (ex) { + // If the reason for the error is that we don't have permission to ask about this process, + // report that as a separate problem. + if (ex.code === 'EPERM') { + throw new Error("Attempted to check whether process " + pid + " was running, but got a permissions error."); + } + return false; + } + } + + /***/ } /******/ ]))); \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-socket.js b/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-socket.js index e59b6ed..c32a882 100644 --- a/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-socket.js +++ b/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-socket.js @@ -44,7 +44,7 @@ /* 0 */ /***/ function(module, exports, __webpack_require__) { - module.exports = __webpack_require__(6); + module.exports = __webpack_require__(7); /***/ }, @@ -124,17 +124,85 @@ /***/ }, /* 6 */ +/***/ function(module, exports) { + + /* + In general, we want the Node child processes to be terminated as soon as the parent .NET processes exit, + because we have no further use for them. If the .NET process shuts down gracefully, it will run its + finalizers, one of which (in OutOfProcessNodeInstance.cs) will kill its associated Node process immediately. + + But if the .NET process is terminated forcefully (e.g., on Linux/OSX with 'kill -9'), then it won't have + any opportunity to shut down its child processes, and by default they will keep running. In this case, it's + up to the child process to detect this has happened and terminate itself. + + There are many possible approaches to detecting when a parent process has exited, most of which behave + differently between Windows and Linux/OS X: + + - On Windows, the parent process can mark its child as being a 'job' that should auto-terminate when + the parent does (http://stackoverflow.com/a/4657392). Not cross-platform. + - The child Node process can get a callback when the parent disconnects (process.on('disconnect', ...)). + But despite http://stackoverflow.com/a/16487966, no callback fires in any case I've tested (Windows / OS X). + - The child Node process can get a callback when its stdin/stdout are disconnected, as described at + http://stackoverflow.com/a/15693934. This works well on OS X, but calling stdout.resume() on Windows + causes the process to terminate prematurely. + - I don't know why, but on Windows, it's enough to invoke process.stdin.resume(). For some reason this causes + the child Node process to exit as soon as the parent one does, but I don't see this documented anywhere. + - You can poll to see if the parent process, or your stdin/stdout connection to it, is gone + - You can directly pass a parent process PID to the child, and then have the child poll to see if it's + still running (e.g., using process.kill(pid, 0), which doesn't kill it but just tests whether it exists, + as per https://nodejs.org/api/process.html#process_process_kill_pid_signal) + - Or, on each poll, you can try writing to process.stdout. If the parent has died, then this will throw. + However I don't see this documented anywhere. It would be nice if you could just poll for whether or not + process.stdout is still connected (without actually writing to it) but I haven't found any property whose + value changes until you actually try to write to it. + + Of these, the only cross-platform approach that is actually documented as a valid strategy is simply polling + to check whether the parent PID is still running. So that's what we do here. + */ + "use strict"; + var pollIntervalMs = 1000; + function exitWhenParentExits(parentPid) { + setInterval(function () { + if (!processExists(parentPid)) { + // Can't log anything at this point, because out stdout was connected to the parent, + // but the parent is gone. + process.exit(); + } + }, pollIntervalMs); + } + exports.exitWhenParentExits = exitWhenParentExits; + function processExists(pid) { + try { + // Sending signal 0 - on all platforms - tests whether the process exists. As long as it doesn't + // throw, that means it does exist. + process.kill(pid, 0); + return true; + } + catch (ex) { + // If the reason for the error is that we don't have permission to ask about this process, + // report that as a separate problem. + if (ex.code === 'EPERM') { + throw new Error("Attempted to check whether process " + pid + " was running, but got a permissions error."); + } + return false; + } + } + + +/***/ }, +/* 7 */ /***/ function(module, exports, __webpack_require__) { "use strict"; // Limit dependencies to core Node modules. This means the code in this file has to be very low-level and unattractive, // but simplifies things for the consumer of this module. __webpack_require__(2); - var net = __webpack_require__(7); + var net = __webpack_require__(8); var path = __webpack_require__(4); - var readline = __webpack_require__(8); + var readline = __webpack_require__(9); var ArgsUtil_1 = __webpack_require__(5); - var virtualConnectionServer = __webpack_require__(9); + var ExitWhenParentExits_1 = __webpack_require__(6); + var virtualConnectionServer = __webpack_require__(10); // Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct // reference to Node's runtime 'require' function. var dynamicRequire = eval('require'); @@ -189,27 +257,28 @@ var parsedArgs = ArgsUtil_1.parseArgs(process.argv); var listenAddress = (useWindowsNamedPipes ? '\\\\.\\pipe\\' : '/tmp/') + parsedArgs.listenAddress; server.listen(listenAddress); + ExitWhenParentExits_1.exitWhenParentExits(parseInt(parsedArgs.parentPid)); -/***/ }, -/* 7 */ -/***/ function(module, exports) { - - module.exports = require("net"); - /***/ }, /* 8 */ /***/ function(module, exports) { - module.exports = require("readline"); + module.exports = require("net"); /***/ }, /* 9 */ +/***/ function(module, exports) { + + module.exports = require("readline"); + +/***/ }, +/* 10 */ /***/ function(module, exports, __webpack_require__) { "use strict"; - var events_1 = __webpack_require__(10); - var VirtualConnection_1 = __webpack_require__(11); + var events_1 = __webpack_require__(11); + var VirtualConnection_1 = __webpack_require__(12); // Keep this in sync with the equivalent constant in the .NET code. Both sides split up their transmissions into frames with this max length, // and both will reject longer frames. var MaxFrameBodyLength = 16 * 1024; @@ -390,13 +459,13 @@ /***/ }, -/* 10 */ +/* 11 */ /***/ function(module, exports) { module.exports = require("events"); /***/ }, -/* 11 */ +/* 12 */ /***/ function(module, exports, __webpack_require__) { "use strict"; @@ -405,7 +474,7 @@ function __() { this.constructor = d; } d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __()); }; - var stream_1 = __webpack_require__(12); + var stream_1 = __webpack_require__(13); /** * Represents a virtual connection. Multiple virtual connections may be multiplexed over a single physical socket connection. */ @@ -446,7 +515,7 @@ /***/ }, -/* 12 */ +/* 13 */ /***/ function(module, exports) { module.exports = require("stream"); diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs index d9ede58..73dcedc 100644 --- a/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs +++ b/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs @@ -114,9 +114,10 @@ If you haven't yet installed node-inspector, you can do so as follows: debuggingArgs = string.Empty; } + var thisProcessPid = Process.GetCurrentProcess().Id; var startInfo = new ProcessStartInfo("node") { - Arguments = debuggingArgs + "\"" + entryPointFilename + "\" " + (commandLineArguments ?? string.Empty), + Arguments = $"{debuggingArgs}\"{entryPointFilename}\" --parentPid {thisProcessPid} {commandLineArguments ?? string.Empty}", UseShellExecute = false, RedirectStandardInput = true, RedirectStandardOutput = true, diff --git a/src/Microsoft.AspNetCore.NodeServices/TypeScript/HttpNodeInstanceEntryPoint.ts b/src/Microsoft.AspNetCore.NodeServices/TypeScript/HttpNodeInstanceEntryPoint.ts index 0e10d99..431810e 100644 --- a/src/Microsoft.AspNetCore.NodeServices/TypeScript/HttpNodeInstanceEntryPoint.ts +++ b/src/Microsoft.AspNetCore.NodeServices/TypeScript/HttpNodeInstanceEntryPoint.ts @@ -4,6 +4,7 @@ import './Util/OverrideStdOutputs'; import * as http from 'http'; import * as path from 'path'; import { parseArgs } from './Util/ArgsUtil'; +import { exitWhenParentExits } from './Util/ExitWhenParentExits'; // Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct // reference to Node's runtime 'require' function. @@ -73,6 +74,8 @@ server.listen(requestedPortOrZero, 'localhost', function () { console.log('[Microsoft.AspNetCore.NodeServices:Listening]'); }); +exitWhenParentExits(parseInt(parsedArgs.parentPid)); + function readRequestBodyAsJson(request, callback) { let requestBodyAsString = ''; request diff --git a/src/Microsoft.AspNetCore.NodeServices/TypeScript/SocketNodeInstanceEntryPoint.ts b/src/Microsoft.AspNetCore.NodeServices/TypeScript/SocketNodeInstanceEntryPoint.ts index 0a6f713..acb5df1 100644 --- a/src/Microsoft.AspNetCore.NodeServices/TypeScript/SocketNodeInstanceEntryPoint.ts +++ b/src/Microsoft.AspNetCore.NodeServices/TypeScript/SocketNodeInstanceEntryPoint.ts @@ -6,6 +6,7 @@ import * as path from 'path'; import * as readline from 'readline'; import { Duplex } from 'stream'; import { parseArgs } from './Util/ArgsUtil'; +import { exitWhenParentExits } from './Util/ExitWhenParentExits'; import * as virtualConnectionServer from './VirtualConnections/VirtualConnectionServer'; // Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct @@ -69,6 +70,8 @@ const parsedArgs = parseArgs(process.argv); const listenAddress = (useWindowsNamedPipes ? '\\\\.\\pipe\\' : '/tmp/') + parsedArgs.listenAddress; server.listen(listenAddress); +exitWhenParentExits(parseInt(parsedArgs.parentPid)); + interface RpcInvocation { moduleName: string; exportedFunctionName: string; diff --git a/src/Microsoft.AspNetCore.NodeServices/TypeScript/Util/ExitWhenParentExits.ts b/src/Microsoft.AspNetCore.NodeServices/TypeScript/Util/ExitWhenParentExits.ts new file mode 100644 index 0000000..672e40b --- /dev/null +++ b/src/Microsoft.AspNetCore.NodeServices/TypeScript/Util/ExitWhenParentExits.ts @@ -0,0 +1,62 @@ +/* +In general, we want the Node child processes to be terminated as soon as the parent .NET processes exit, +because we have no further use for them. If the .NET process shuts down gracefully, it will run its +finalizers, one of which (in OutOfProcessNodeInstance.cs) will kill its associated Node process immediately. + +But if the .NET process is terminated forcefully (e.g., on Linux/OSX with 'kill -9'), then it won't have +any opportunity to shut down its child processes, and by default they will keep running. In this case, it's +up to the child process to detect this has happened and terminate itself. + +There are many possible approaches to detecting when a parent process has exited, most of which behave +differently between Windows and Linux/OS X: + + - On Windows, the parent process can mark its child as being a 'job' that should auto-terminate when + the parent does (http://stackoverflow.com/a/4657392). Not cross-platform. + - The child Node process can get a callback when the parent disconnects (process.on('disconnect', ...)). + But despite http://stackoverflow.com/a/16487966, no callback fires in any case I've tested (Windows / OS X). + - The child Node process can get a callback when its stdin/stdout are disconnected, as described at + http://stackoverflow.com/a/15693934. This works well on OS X, but calling stdout.resume() on Windows + causes the process to terminate prematurely. + - I don't know why, but on Windows, it's enough to invoke process.stdin.resume(). For some reason this causes + the child Node process to exit as soon as the parent one does, but I don't see this documented anywhere. + - You can poll to see if the parent process, or your stdin/stdout connection to it, is gone + - You can directly pass a parent process PID to the child, and then have the child poll to see if it's + still running (e.g., using process.kill(pid, 0), which doesn't kill it but just tests whether it exists, + as per https://nodejs.org/api/process.html#process_process_kill_pid_signal) + - Or, on each poll, you can try writing to process.stdout. If the parent has died, then this will throw. + However I don't see this documented anywhere. It would be nice if you could just poll for whether or not + process.stdout is still connected (without actually writing to it) but I haven't found any property whose + value changes until you actually try to write to it. + +Of these, the only cross-platform approach that is actually documented as a valid strategy is simply polling +to check whether the parent PID is still running. So that's what we do here. +*/ + +const pollIntervalMs = 1000; + +export function exitWhenParentExits(parentPid: number) { + setInterval(() => { + if (!processExists(parentPid)) { + // Can't log anything at this point, because out stdout was connected to the parent, + // but the parent is gone. + process.exit(); + } + }, pollIntervalMs); +} + +function processExists(pid: number) { + try { + // Sending signal 0 - on all platforms - tests whether the process exists. As long as it doesn't + // throw, that means it does exist. + process.kill(pid, 0); + return true; + } catch (ex) { + // If the reason for the error is that we don't have permission to ask about this process, + // report that as a separate problem. + if (ex.code === 'EPERM') { + throw new Error(`Attempted to check whether process ${pid} was running, but got a permissions error.`); + } + + return false; + } +}