Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

fix: signal handling #227

Merged
merged 2 commits into from Mar 3, 2020
Merged

fix: signal handling #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f730e45
fix: signal handling
Feb 13, 2020
03e2cf7
Merge branch 'master' into fix/sigint-handling
kentcdodds Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
128 changes: 93 additions & 35 deletions src/__tests__/index.js
View file
Open in desktop
Expand Up @@ -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()
})
Expand Down Expand Up @@ -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'])
Expand All @@ -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) {
Expand Down
65 changes: 52 additions & 13 deletions src/index.js
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -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')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signal event receives the signal name as the argument. That means you don't need to create a special handler for each signal:

const delegateSignalToChild = signal => {
  process.kill(child.pid, signal);
}
process.on('SIGTERM', delegateSignalToChild);
process.on('SIGBREAK', delegateSignalToChild);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I was following other examples around the place to get the correct handling working.

As the original code created inline arrow functions to handle the individual events I just kept a similar code flow.

I wasn't sure whether the listeners would vary or be removed independently so went with the more verbose style.

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that the code does the following?

  • when the parent process (cross-env) receives a SIGTERM signal, it will handle it by sending it to the child process
  • then when the child process exits and its exit status says it's been killed with a signal, the parent sends that signal to itself?
  • that means that the parent process receives the signal two times: first from external source, handled by a custom handler that forwards it to the child, second time from itself, and handled by the default handler, as the custom handler was removed a moment ago?
  • the second SIGTERM, with custom handler removed, will cause the parent process to terminate, as that's what the default handler does?
  • the parent process will end with an exit status that says it was terminated with SIGTERM?

The SIGINT handling seems to be a bit special: the parent process' custom handler simply ignores it, and relies on the fact that the child process received the SIGINT signal, too. Then in the child.on('exit') callback, it will uninstall the "ignore" handler and will send SIGINT to itself. Then it will finally terminate with SIGINT signal exit status.

One case that looks a bit suspicious: if I kill the parent process explicitly, with kill -s SIGINT <parent-pid>, then it will ignore the signal and do nothing. Both processes continue to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes your understanding is correct for the example using SIGTERM and all other non-SIGINT signals that are listened for.

For the example of SIGINT, all I can say is the documentation I found indicates this is what happens on Unix systems. I haven't tested this on that platform. If you can find a definitive guide to signal handling I"m happy to adjust the code.

On Windows there is no such thing as signals, they are emulated by Node Signal Events

Did you try the kill -s SIGINT <parent-pid>? As I'm on windows I dont have those utilities.

}
process.exit(crossEnvExitCode) //eslint-disable-line no-process-exit
})
return proc
return child
}
return null
}
Expand Down