From 8a9cf0e4de5dba2c990ff9bee77bf65cb3fc2c8d Mon Sep 17 00:00:00 2001 From: Barrie Treloar Date: Wed, 4 Mar 2020 05:05:18 +1030 Subject: [PATCH] fix: signal handling (#227) Signals should delegate to the child process to determine what to do as cross-env is a facade to spawning them cross platform. SIGINT, in particular, can decide swallow the signal and continue on. cross-env needs to wait for the child to decide when it's time to exit. fixed leaking `process.on` listeners. Co-authored-by: Kent C. Dodds --- src/__tests__/index.js | 128 ++++++++++++++++++++++++++++++----------- src/index.js | 65 ++++++++++++++++----- 2 files changed, 145 insertions(+), 48 deletions(-) diff --git a/src/__tests__/index.js b/src/__tests__/index.js index 0d5abae..c891dda 100644 --- a/src/__tests__/index.js +++ b/src/__tests__/index.js @@ -6,16 +6,44 @@ jest.mock('cross-spawn') const crossEnv = require('../') -const getSpawned = (call = 0) => crossSpawnMock.spawn.mock.results[call].value +/* + Each test should spawn at most once. + */ +const getSpawned = () => { + if (crossSpawnMock.spawn.mock.results.length !== 0) { + return crossSpawnMock.spawn.mock.results[0].value + } + + return undefined +} + +const getSpawnedOnExitCallBack = () => getSpawned().on.mock.calls[0][1] -process.setMaxListeners(20) +// Enforce checks for leaking process.on() listeners in cross-env +process.setMaxListeners(1) beforeEach(() => { jest.spyOn(process, 'exit').mockImplementation(() => {}) - crossSpawnMock.spawn.mockReturnValue({on: jest.fn(), kill: jest.fn()}) + jest.spyOn(process, 'kill').mockImplementation(() => {}) + crossSpawnMock.spawn.mockReturnValue({ + pid: 42, + on: jest.fn(), + kill: jest.fn(), + }) }) afterEach(() => { + // stop tests from leaking process.on() listeners in cross-env + const spawned = getSpawned() + if (spawned) { + const spawnExitCallback = getSpawnedOnExitCallBack() + const signal = 'SIGTEST' + const exitCode = null + spawnExitCallback(exitCode, signal) + + process.removeAllListeners('exit') + } + jest.clearAllMocks() process.exit.mockRestore() }) @@ -130,20 +158,6 @@ test(`does not normalize command arguments on windows`, () => { ) }) -test(`propagates kill signals`, () => { - testEnvSetting({FOO_ENV: 'foo=bar'}, 'FOO_ENV="foo=bar"') - - process.emit('SIGTERM') - process.emit('SIGINT') - process.emit('SIGHUP') - process.emit('SIGBREAK') - const spawned = getSpawned() - expect(spawned.kill).toHaveBeenCalledWith('SIGTERM') - expect(spawned.kill).toHaveBeenCalledWith('SIGINT') - expect(spawned.kill).toHaveBeenCalledWith('SIGHUP') - expect(spawned.kill).toHaveBeenCalledWith('SIGBREAK') -}) - test(`keeps backslashes`, () => { isWindowsMock.mockReturnValue(true) crossEnv(['echo', '\\\\\\\\someshare\\\\somefolder']) @@ -157,29 +171,73 @@ test(`keeps backslashes`, () => { ) }) -test(`propagates unhandled exit signal`, () => { - const {spawned} = testEnvSetting({FOO_ENV: 'foo=bar'}, 'FOO_ENV="foo=bar"') - const spawnExitCallback = spawned.on.mock.calls[0][1] - const spawnExitCode = null - spawnExitCallback(spawnExitCode) - expect(process.exit).toHaveBeenCalledWith(1) +describe(`cross-env delegates signals to spawn`, () => { + test(`SIGINT is not delegated`, () => { + const signal = 'SIGINT' + + crossEnv(['echo', 'hello world']) + const spawnExitCallback = getSpawnedOnExitCallBack() + const exitCode = null + const parentProcessId = expect.any(Number) + + // Parent receives signal + // SIGINT is sent to all processes in group, no need to delegated. + process.emit(signal) + expect(process.kill).not.toHaveBeenCalled() + // child handles signal and 'exits' + spawnExitCallback(exitCode, signal) + expect(process.kill).toHaveBeenCalledTimes(1) + expect(process.kill).toHaveBeenCalledWith(parentProcessId, signal) + }) + + test.each(['SIGTERM', 'SIGBREAK', 'SIGHUP', 'SIGQUIT'])( + `delegates %s`, + signal => { + crossEnv(['echo', 'hello world']) + const spawnExitCallback = getSpawnedOnExitCallBack() + const exitCode = null + const parentProcessId = expect.any(Number) + + // Parent receives signal + process.emit(signal) + expect(process.kill).toHaveBeenCalledTimes(1) + expect(process.kill).toHaveBeenCalledWith(42, signal) + // Parent delegates signal to child, child handles signal and 'exits' + spawnExitCallback(exitCode, signal) + expect(process.kill).toHaveBeenCalledTimes(2) + expect(process.kill).toHaveBeenCalledWith(parentProcessId, signal) + }, + ) }) -test(`exits cleanly with SIGINT with a null exit code`, () => { - const {spawned} = testEnvSetting({FOO_ENV: 'foo=bar'}, 'FOO_ENV="foo=bar"') - const spawnExitCallback = spawned.on.mock.calls[0][1] - const spawnExitCode = null - const spawnExitSignal = 'SIGINT' - spawnExitCallback(spawnExitCode, spawnExitSignal) - expect(process.exit).toHaveBeenCalledWith(0) +describe(`spawn received signal and exits`, () => { + test.each(['SIGTERM', 'SIGINT', 'SIGBREAK', 'SIGHUP', 'SIGQUIT'])( + `delegates %s`, + signal => { + crossEnv(['echo', 'hello world']) + + const spawnExitCallback = getSpawnedOnExitCallBack() + const exitCode = null + const parentProcessId = expect.any(Number) + + // cross-env child.on('exit') re-raises signal, now with no signal handlers + spawnExitCallback(exitCode, signal) + process.emit('exit', exitCode, signal) + expect(process.kill).toHaveBeenCalledTimes(1) + expect(process.kill).toHaveBeenCalledWith(parentProcessId, signal) + }, + ) }) -test(`propagates regular exit code`, () => { - const {spawned} = testEnvSetting({FOO_ENV: 'foo=bar'}, 'FOO_ENV="foo=bar"') - const spawnExitCallback = spawned.on.mock.calls[0][1] +test(`spawn regular exit code`, () => { + crossEnv(['echo', 'hello world']) + + const spawnExitCallback = getSpawnedOnExitCallBack() const spawnExitCode = 0 - spawnExitCallback(spawnExitCode) - expect(process.exit).toHaveBeenCalledWith(0) + const spawnExitSignal = null + spawnExitCallback(spawnExitCode, spawnExitSignal) + expect(process.exit).not.toHaveBeenCalled() + expect(process.exitCode).toBe(0) }) function testEnvSetting(expected, ...envSettings) { diff --git a/src/index.js b/src/index.js index d9bd227..2e5011b 100644 --- a/src/index.js +++ b/src/index.js @@ -10,7 +10,7 @@ function crossEnv(args, options = {}) { const [envSetters, command, commandArgs] = parseCommand(args) const env = getEnvVars(envSetters) if (command) { - const proc = spawn( + let child = spawn( // run `path.normalize` for command(on windows) commandConvert(command, env, true), // by default normalize is `false`, so not run for cmd args @@ -21,20 +21,59 @@ function crossEnv(args, options = {}) { env, }, ) - process.on('SIGTERM', () => proc.kill('SIGTERM')) - process.on('SIGINT', () => proc.kill('SIGINT')) - process.on('SIGBREAK', () => proc.kill('SIGBREAK')) - process.on('SIGHUP', () => proc.kill('SIGHUP')) - proc.on('exit', (code, signal) => { - let crossEnvExitCode = code - // exit code could be null when OS kills the process(out of memory, etc) or due to node handling it - // but if the signal is SIGINT the user exited the process so we want exit code 0 - if (crossEnvExitCode === null) { - crossEnvExitCode = signal === 'SIGINT' ? 0 : 1 + + // See https://github.com/jtlapp/node-cleanup + // + // > When you hit Ctrl-C, you send a SIGINT signal to each process in the + // > current process group. A process group is set of processes that are + // > all supposed to end together as a group instead of persisting + // > independently. However, some programs, such as Emacs, intercept and + // > repurpose SIGINT so that it does not end the process. In such cases, + // > SIGINT should not end any processes of the group. + // + // Delegate decision to terminate to child process, if the child exits on + // SIGINT then the `child.on('exit')` callback will be invoked, re-raising + // the signal to the parent process + const delegateSignalToChild = signal => () => { + // SIGINT is sent to all processes in group, no need to delegate. + if (child && signal !== 'SIGINT') { + process.kill(child.pid, signal) + } + } + + const sigtermHandler = delegateSignalToChild('SIGTERM') + const sigintHandler = delegateSignalToChild('SIGINT') + const sigbreakHandler = delegateSignalToChild('SIGBREAK') + const sighupHandler = delegateSignalToChild('SIGHUP') + const sigquitHandler = delegateSignalToChild('SIGQUIT') + + process.on('SIGTERM', sigtermHandler) + process.on('SIGINT', sigintHandler) + process.on('SIGBREAK', sigbreakHandler) + process.on('SIGHUP', sighupHandler) + process.on('SIGQUIT', sigquitHandler) + + child.on('exit', (exitCode, signal) => { + // Child has decided to exit. + child = null + process.removeListener('SIGTERM', sigtermHandler) + process.removeListener('SIGINT', sigintHandler) + process.removeListener('SIGBREAK', sigbreakHandler) + process.removeListener('SIGHUP', sighupHandler) + process.removeListener('SIGQUIT', sigquitHandler) + + if (exitCode !== null) { + // Calling process.exit is not necessary most of the time, + // see https://nodejs.org/api/process.html#process_process_exit_code + process.exitCode = exitCode + } + if (signal !== null) { + // Pass through child's signal to parent. + // SIGINT should not be transformed into a 0 exit code + process.kill(process.pid, signal) } - process.exit(crossEnvExitCode) //eslint-disable-line no-process-exit }) - return proc + return child } return null }