From 5046af5313396f93cbca68969d4c804afd757605 Mon Sep 17 00:00:00 2001 From: Jonas Hermsmeier Date: Wed, 15 Nov 2017 15:52:57 +0100 Subject: [PATCH] fix(child-writer): Handle exits due to a signal (#1843) This adds handling for cases where the writer child process exits due to reception of a signal, while also adjusting some peripherals, like the IPC socket directory and inherited process environment. Another addition is that the child process is explicitly killed should an error arise on the IPC. Change-Type: patch --- lib/child-writer/constants.js | 8 ++ lib/child-writer/index.js | 26 ++++--- lib/child-writer/writer-proxy.js | 129 ++++++++++++++++++++----------- 3 files changed, 107 insertions(+), 56 deletions(-) diff --git a/lib/child-writer/constants.js b/lib/child-writer/constants.js index 34b0ab3f..86aca7bf 100644 --- a/lib/child-writer/constants.js +++ b/lib/child-writer/constants.js @@ -17,6 +17,7 @@ 'use strict' const path = require('path') +const os = require('os') /** * @summary Child writer constants @@ -25,6 +26,13 @@ const path = require('path') */ module.exports = { + /** + * @property {String} TMP_DIRECTORY + * @memberof CONSTANTS + * @constant + */ + TMP_DIRECTORY: process.env.XDG_RUNTIME_DIR || os.tmpdir(), + /** * @property {String} PROJECT_ROOT * @memberof CONSTANTS diff --git a/lib/child-writer/index.js b/lib/child-writer/index.js index 20821b81..e627e20b 100644 --- a/lib/child-writer/index.js +++ b/lib/child-writer/index.js @@ -26,6 +26,16 @@ const CONSTANTS = require('./constants') const EXIT_CODES = require('../shared/exit-codes') const robot = require('../shared/robot') +// There might be multiple Etcher instances running at +// the same time, therefore we must ensure each IPC +// server/client has a different name. +process.env.IPC_SERVER_ID = `etcher-server-${process.pid}` +process.env.IPC_CLIENT_ID = `etcher-client-${process.pid}` + +ipc.config.id = process.env.IPC_SERVER_ID +ipc.config.socketRoot = CONSTANTS.TMP_DIRECTORY +ipc.config.silent = true + /** * @summary Perform a write * @function @@ -67,14 +77,6 @@ exports.write = (image, drive, options) => { unmountOnSuccess: options.unmountOnSuccess }) - // There might be multiple Etcher instances running at - // the same time, therefore we must ensure each IPC - // server/client has a different name. - process.env.IPC_SERVER_ID = `etcher-server-${process.pid}` - process.env.IPC_CLIENT_ID = `etcher-client-${process.pid}` - - ipc.config.id = process.env.IPC_SERVER_ID - ipc.config.silent = true ipc.serve() /** @@ -192,7 +194,7 @@ exports.write = (image, drive, options) => { child.on('error', emitError) - child.on('close', (code) => { + child.on('exit', (code, signal) => { terminateServer() if (code === EXIT_CODES.CANCELLED) { @@ -207,7 +209,11 @@ exports.write = (image, drive, options) => { return null } - return emitError(new Error(`Child process exited with error code: ${code}`)) + const error = new Error(`Child process exited with code ${code}, signal ${signal}`) + error.code = code + error.signal = signal + + return emitError(error) }) }) diff --git a/lib/child-writer/writer-proxy.js b/lib/child-writer/writer-proxy.js index 6ed458ce..ddb25d69 100644 --- a/lib/child-writer/writer-proxy.js +++ b/lib/child-writer/writer-proxy.js @@ -27,6 +27,9 @@ const EXIT_CODES = require('../shared/exit-codes') const robot = require('../shared/robot') const permissions = require('../shared/permissions') const packageJSON = require('../../package.json') +const CONSTANTS = require('./constants') + +/* eslint-disable no-eq-null */ // This script is in charge of spawning the writer process and // ensuring it has the necessary privileges. It might look a bit @@ -64,6 +67,18 @@ const OPTIONS_INDEX_START = 2 */ const etcherArguments = process.argv.slice(OPTIONS_INDEX_START) +ipc.config.id = process.env.IPC_CLIENT_ID +ipc.config.socketRoot = CONSTANTS.TMP_DIRECTORY +ipc.config.silent = true + +// > If set to 0, the client will NOT try to reconnect. +// See https://github.com/RIAEvangelist/node-ipc/ +// +// The purpose behind this change is for this process +// to emit a "disconnect" event as soon as the GUI +// process is closed, so we can kill the CLI as well. +ipc.config.stopRetrying = 0 + permissions.isElevated().then((elevated) => { if (!elevated) { console.log('Attempting to elevate') @@ -109,62 +124,84 @@ permissions.isElevated().then((elevated) => { console.log('Re-spawning with elevation') return new Bluebird((resolve, reject) => { - ipc.config.id = process.env.IPC_CLIENT_ID - ipc.config.silent = true + let child = null - // > If set to 0, the client will NOT try to reconnect. - // See https://github.com/RIAEvangelist/node-ipc/ - // - // The purpose behind this change is for this process - // to emit a "disconnect" event as soon as the GUI - // process is closed, so we can kill the CLI as well. - ipc.config.stopRetrying = 0 + /** + * @summary Emit an object message to the IPC server + * @function + * @private + * + * @param {Buffer} data - json message data + * + * @example + * emitMessage(Buffer.from(JSON.stringify({ + * foo: 'bar' + * }))); + */ + const emitMessage = (data) => { + // Output from stdout/stderr coming from the CLI might be buffered, + // causing several progress lines to come up at once as single message. + // Trying to parse multiple JSON objects separated by new lines will + // of course make the parser confused, causing errors later on. + _.each(utils.splitObjectLines(data.toString()), (object) => { + ipc.of[process.env.IPC_SERVER_ID].emit('message', object) + }) + } + + /** + * @summary Emit an error message over the IPC and shut down the child + * @function + * @private + * @param {Error} error - error + * @example + * onError(error) + */ + const onError = (error) => { + ipc.of[process.env.IPC_SERVER_ID].emit('message', { + error: error.message, + data: error.stack + }) + child && child.kill() + reject(error) + } ipc.connectTo(process.env.IPC_SERVER_ID, () => { - ipc.of[process.env.IPC_SERVER_ID].on('error', reject) + ipc.of[process.env.IPC_SERVER_ID].on('error', onError) + ipc.of[process.env.IPC_SERVER_ID].on('disconnect', () => { + onError(new Error('Writer process disconnected')) + }) + ipc.of[process.env.IPC_SERVER_ID].on('connect', () => { - const child = childProcess.spawn(executable, etcherArguments, { - env: { + // Inherit the parent evnironment + const childEnv = _.assign({}, process.env, { - // The CLI might call operating system utilities (like `diskutil`), - // so we must ensure the `PATH` is inherited. - PATH: process.env.PATH, + // The CLI might call operating system utilities (like `diskutil`), + // so we must ensure the `PATH` is inherited. + PATH: process.env.PATH, - ELECTRON_RUN_AS_NODE: 1, - ETCHER_CLI_ROBOT: 1, + ELECTRON_RUN_AS_NODE: 1, + ETCHER_CLI_ROBOT: 1, - // Enable extra logging from mountutils - // See https://github.com/resin-io-modules/mountutils - MOUNTUTILS_DEBUG: 1 - - } + // Enable extra logging from mountutils + // See https://github.com/resin-io-modules/mountutils + MOUNTUTILS_DEBUG: 1 }) - ipc.of[process.env.IPC_SERVER_ID].on('disconnect', _.bind(child.kill, child)) - child.on('error', reject) - child.on('close', resolve) + child = childProcess.spawn(executable, etcherArguments, { + env: childEnv + }) - /** - * @summary Emit an object message to the IPC server - * @function - * @private - * - * @param {Buffer} data - json message data - * - * @example - * emitMessage(Buffer.from(JSON.stringify({ - * foo: 'bar' - * }))); - */ - const emitMessage = (data) => { - // Output from stdout/stderr coming from the CLI might be buffered, - // causing several progress lines to come up at once as single message. - // Trying to parse multiple JSON objects separated by new lines will - // of course make the parser confused, causing errors later on. - _.each(utils.splitObjectLines(data.toString()), (object) => { - ipc.of[process.env.IPC_SERVER_ID].emit('message', object) - }) - } + child.on('error', onError) + child.on('exit', (code, signal) => { + if (code != null && signal == null) { + resolve(code) + } else { + const error = new Error(`Exited with code ${code}, signal ${signal}`) + error.code = code + error.signal = signal + reject(error) + } + }) child.stdout.on('data', emitMessage) child.stderr.on('data', emitMessage)