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 <[email protected]>

Author: baerrach

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) {

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
 }