From 2a1f44c9053702fdc3fffca38afc041cbf634a05 Mon Sep 17 00:00:00 2001 From: "Kent C. Dodds" Date: Thu, 5 Mar 2020 13:29:17 -0700 Subject: [PATCH] Revert "fix: signal handling (#227)" This reverts commit 8a9cf0e4de5dba2c990ff9bee77bf65cb3fc2c8d. --- src/__tests__/index.js | 128 +++++++++++------------------------------ src/index.js | 65 +++++---------------- 2 files changed, 48 insertions(+), 145 deletions(-) diff --git a/src/__tests__/index.js b/src/__tests__/index.js index c891dda..0d5abae 100644 --- a/src/__tests__/index.js +++ b/src/__tests__/index.js @@ -6,44 +6,16 @@ jest.mock('cross-spawn') const crossEnv = require('../') -/* - 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] +const getSpawned = (call = 0) => crossSpawnMock.spawn.mock.results[call].value -// Enforce checks for leaking process.on() listeners in cross-env -process.setMaxListeners(1) +process.setMaxListeners(20) beforeEach(() => { jest.spyOn(process, 'exit').mockImplementation(() => {}) - jest.spyOn(process, 'kill').mockImplementation(() => {}) - crossSpawnMock.spawn.mockReturnValue({ - pid: 42, - on: jest.fn(), - kill: jest.fn(), - }) + crossSpawnMock.spawn.mockReturnValue({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() }) @@ -158,6 +130,20 @@ 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']) @@ -171,73 +157,29 @@ test(`keeps backslashes`, () => { ) }) -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(`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(`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(`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) }) -test(`spawn regular exit code`, () => { - crossEnv(['echo', 'hello world']) - - const spawnExitCallback = getSpawnedOnExitCallBack() +test(`propagates regular exit code`, () => { + const {spawned} = testEnvSetting({FOO_ENV: 'foo=bar'}, 'FOO_ENV="foo=bar"') + const spawnExitCallback = spawned.on.mock.calls[0][1] const spawnExitCode = 0 - const spawnExitSignal = null - spawnExitCallback(spawnExitCode, spawnExitSignal) - expect(process.exit).not.toHaveBeenCalled() - expect(process.exitCode).toBe(0) + spawnExitCallback(spawnExitCode) + expect(process.exit).toHaveBeenCalledWith(0) }) function testEnvSetting(expected, ...envSettings) { diff --git a/src/index.js b/src/index.js index 2e5011b..d9bd227 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) { - let child = spawn( + const proc = spawn( // run `path.normalize` for command(on windows) commandConvert(command, env, true), // by default normalize is `false`, so not run for cmd args @@ -21,59 +21,20 @@ function crossEnv(args, options = {}) { env, }, ) - - // 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.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 } + process.exit(crossEnvExitCode) //eslint-disable-line no-process-exit }) - return child + return proc } return null }