Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature to retry process killing after specified duration #208

Merged
merged 51 commits into from Jun 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
93b8063
Add feature to retry process killing after specified duration
zokker13 Apr 28, 2019
9b8efe9
Fixup: repair code according to styleguide
zokker13 Apr 28, 2019
fcdcaba
Fixup: Return kill result from wrapper
zokker13 Apr 28, 2019
3ea8c34
Fixup: Don't kill without reason
zokker13 Apr 29, 2019
13c3ed1
Fixup: Impove signal checking based on types and node defaults
zokker13 Apr 29, 2019
6bf3aea
Fixup: Increase default timeout for process termination
zokker13 Apr 29, 2019
90d72a1
Fixup: Add possibility to disable the timeout
zokker13 Apr 29, 2019
856896d
Fixup: Imporove timeout behavior by unref'ing
zokker13 Apr 29, 2019
cff5e2f
Fixup: Move retryAfter to more appropriate place
zokker13 May 1, 2019
250498f
Fixup: Remove wrong comment
zokker13 May 1, 2019
fc0d7b2
Fixup: call `originalKill` only once
zokker13 May 1, 2019
20e5ecf
Add nice documentation for the new behavior
zokker13 May 1, 2019
d2963e5
Add broken tests that need more care
zokker13 May 1, 2019
f6526aa
Fixup: Improve the readme and follow suggestions
zokker13 May 3, 2019
91f755d
Fixup: Remove redundant sentence
zokker13 May 3, 2019
371df69
Fixup: Remove redundant asserts to `proc.pid`
zokker13 May 3, 2019
702cbf3
Fixup: Impove tests and align them to the others
zokker13 May 3, 2019
a84be97
Fixup: Improve readme with proper formatting
zokker13 May 5, 2019
3cb8dd0
Fixup: Use shallow comparison and simplify tests
zokker13 May 5, 2019
904154a
Add proper typescript definitions to the new function
zokker13 May 5, 2019
c6c3873
Fixup: Repair the tests and wait for proper state of the subprocess
zokker13 May 5, 2019
7b00bbd
Fixup: Avoid direct Promise building and promisify
zokker13 May 5, 2019
83345ad
Fixup: Repair tests on Windows (add conditionals in test)
zokker13 May 6, 2019
068fbb6
Fixup: Skip tests if we are on Windows
zokker13 May 15, 2019
c2da4d2
Fixup: Improve readme
zokker13 May 15, 2019
61e2491
Fixup: Make test code better readable
zokker13 May 15, 2019
f024c12
Fixup: Rename fixture to something without `sub-process`
zokker13 May 15, 2019
ef944a6
Fixup: Combine test runs with win32 exclusion
zokker13 May 16, 2019
22061ec
Fixup: Reorder tests and drop simple function
zokker13 May 16, 2019
c9ddaa6
Fixup: Make readme compatible with new style of wording
zokker13 May 16, 2019
b9ab61f
Fixup: Reorder types and add tsdoc
zokker13 May 16, 2019
785c7cf
Fixup: Remove useless spaces
zokker13 May 16, 2019
6345ca0
Fixup: Rephrase console message for fixture
zokker13 May 18, 2019
d3718d8
Fixup: Move ts type definitions to proper nnamespace
zokker13 May 18, 2019
141cb53
Fixup: Add default options, remove string as number
zokker13 May 18, 2019
75fe83c
Fixup: Improve tsdoc wording
zokker13 May 18, 2019
86fd58f
Fixup: Rename option fields
zokker13 May 18, 2019
ed7feb5
Fixup: Improve test definition wording
zokker13 May 18, 2019
fa1502c
Fixup: proper description why win32 exclusion in test
zokker13 May 18, 2019
7bf47ad
Fixup: Follow readme suggestions
zokker13 May 18, 2019
7f9c122
Add test code for ts types
zokker13 May 18, 2019
0fca4a1
Fixup: Optimize code chunks
zokker13 May 21, 2019
5615e22
Fixup: Add old tests that were remove on accident
zokker13 May 21, 2019
02f5ab1
Fixup: improve ts tests I suppose
zokker13 May 21, 2019
8fc7179
Fixup: More optimization for kill function
zokker13 May 21, 2019
bb53e7f
Fixup: Extract complicated if statement to own function
zokker13 May 23, 2019
50d4219
Fixup: Add defaults in tsdoc
zokker13 May 23, 2019
cb8aacb
Fixup: Undo previously added testchange of unrelated code
zokker13 May 23, 2019
756cb78
Fixup: Undo readme change
zokker13 May 23, 2019
454f020
Update index.d.ts
sindresorhus Jun 1, 2019
4c1af51
Update readme.md
sindresorhus Jun 1, 2019
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
12 changes: 12 additions & 0 deletions fixtures/no-killable
@@ -0,0 +1,12 @@
#!/usr/bin/env node
'use strict';

process.on('SIGTERM', () => {
console.log('Received SIGTERM, but we ignore it');
});

process.send('');

setInterval(() => {
// Run forever
}, 20000);
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
21 changes: 21 additions & 0 deletions index.d.ts
Expand Up @@ -279,11 +279,32 @@ declare namespace execa {
isCanceled: boolean;
}

interface KillOptions {
/**
If the first signal does not terminate the child process after a specified timeout, a `SIGKILL` signal will be sent to the process.

@default true
*/
forceKill?: boolean;
ehmicky marked this conversation as resolved.
Show resolved Hide resolved

/**
Milliseconds to wait for the child process to terminate before sending a `SIGKILL` signal.

@default 5000
*/
forceKillAfter?: number;
}

interface ExecaChildPromise<StdoutErrorType> {
catch<ResultType = never>(
onRejected?: (reason: ExecaError<StdoutErrorType>) => ResultType | PromiseLike<ResultType>
): Promise<ExecaReturnValue<StdoutErrorType> | ResultType>;

/**
Same as the original [`child_process#kill()`](https://nodejs.org/api/child_process.html#child_process_subprocess_kill_signal), except if `signal` is `SIGTERM` (the default value) and the child process is not terminated after 5 seconds, force it by sending `SIGKILL`.
*/
kill(signal?: string, options?: execa.KillOptions): void;

/**
Similar to [`childProcess.kill()`](https://nodejs.org/api/child_process.html#child_process_subprocess_kill_signal). This is preferred when cancelling the child process execution as the error is more descriptive and [`childProcessResult.isCanceled`](#iscanceled) is set to `true`.
*/
Expand Down
25 changes: 25 additions & 0 deletions index.js
Expand Up @@ -209,6 +209,14 @@ function joinCommand(file, args = []) {
return [file, ...args].join(' ');
}

function shouldForceKill(signal, options, killResult) {
return ((typeof signal === 'string' &&
signal.toUpperCase() === 'SIGTERM') ||
signal === os.constants.signals.SIGTERM) &&
options.forceKill !== false &&
killResult;
}

const execa = (file, args, options) => {
const parsed = handleArgs(file, args, options);
const {encoding, buffer, maxBuffer} = parsed.options;
Expand All @@ -221,6 +229,23 @@ const execa = (file, args, options) => {
return Promise.reject(error);
}

const originalKill = spawned.kill.bind(spawned);
spawned.kill = (signal = 'SIGTERM', options = {}) => {
const killResult = originalKill(signal);
if (shouldForceKill(signal, options, killResult)) {
const forceKillAfter = Number.isInteger(options.forceKillAfter) ?
options.forceKillAfter :
5000;
setTimeout(() => {
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
try {
originalKill('SIGKILL');
} catch (_) {}
}, forceKillAfter).unref();
}

return killResult;
};

// #115
let removeExitHandler;
if (parsed.options.cleanup && !parsed.options.detached) {
Expand Down
8 changes: 8 additions & 0 deletions index.test-d.ts
Expand Up @@ -119,6 +119,14 @@ execa('unicorns', {maxBuffer: 1000});
execa('unicorns', {killSignal: 'SIGTERM'});
execa('unicorns', {killSignal: 9});
execa('unicorns', {windowsVerbatimArguments: true});
execa('unicorns').kill();
execa('unicorns').kill('SIGKILL');
execa('unicorns').kill(undefined);
execa('unicorns').kill('SIGKILL', {});
execa('unicorns').kill('SIGKILL', {forceKill: true});
execa('unicorns').kill('SIGKILL', {forceKill: false});
execa('unicorns').kill('SIGKILL', {forceKillAfter: 42});
execa('unicorns').kill('SIGKILL', {forceKillAfter: undefined});

expectType<ExecaChildProcess<string>>(execa('unicorns'));
expectType<ExecaReturnValue<string>>(await execa('unicorns'));
Expand Down
27 changes: 26 additions & 1 deletion readme.md
Expand Up @@ -112,8 +112,15 @@ try {
}
*/
}
```

// Kill a process with SIGTERM, and after 2 seconds, kill it with SIGKILL
const subprocess = execa('node');
setTimeout(() => {
subprocess.kill('SIGTERM', {
forceKillAfter: 2000
});
}, 1000);
```

## API

Expand All @@ -129,6 +136,24 @@ Returns a [`child_process` instance](https://nodejs.org/api/child_process.html#c
- is also a `Promise` resolving or rejecting with a [`childProcessResult`](#childProcessResult).
- exposes the following additional methods and properties.

#### kill([signal], [options])

Same as the original [`child_process#kill()`](https://nodejs.org/api/child_process.html#child_process_subprocess_kill_signal) except: if `signal` is `SIGTERM` (the default value) and the child process is not terminated after 5 seconds, force it by sending `SIGKILL`.

##### options.forceKill

Type: `boolean`<br>
Default: `true`

If the first signal does not terminate the child process after a specified timeout, a `SIGKILL` signal will be sent to the process.

##### options.forceKillAfter

Type: `string`<br>
Default: `5000`
Copy link
Owner

Choose a reason for hiding this comment

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

What considerations were given for having 5000 as the default?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Originally @zokker13 had it as 2000. I suggested 5000 because I feel like 2 seconds is not enough to do proper shutdown of some processes. E.g. some servers need to perform some network requests before properly closing.

But that's an edge case, so this sounds good if you want a much smaller default value. Which one do you have in mind?


Milliseconds to wait for the child process to terminate before sending `SIGKILL`.

#### cancel()

Similar to [`childProcess.kill()`](https://nodejs.org/api/child_process.html#child_process_subprocess_kill_signal). This is preferred when cancelling the child process execution as the error is more descriptive and [`childProcessResult.isCanceled`](#iscanceled) is set to `true`.
Expand Down
61 changes: 61 additions & 0 deletions test.js
Expand Up @@ -118,6 +118,67 @@ test('skip throwing when using reject option in sync mode', t => {
t.is(exitCode, 2);
});

test('execa() with .kill() after it with SIGKILL should kill cleanly', async t => {
const subprocess = execa('node', ['fixtures/no-killable'], {
stdio: ['ipc']
});

await pEvent(subprocess, 'message');

subprocess.kill('SIGKILL');

const {signal} = await t.throwsAsync(subprocess);
t.is(signal, 'SIGKILL');
});

// `SIGTERM` cannot be caught on Windows, and it always aborts the process (like `SIGKILL` on Unix).
// Therefore, this feature and those tests do not make sense on Windows.
if (process.platform !== 'win32') {
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
test('execa() with .kill() after it with SIGTERM should not kill (no retry)', async t => {
const subprocess = execa('node', ['fixtures/no-killable'], {
stdio: ['ipc']
});

await pEvent(subprocess, 'message');

subprocess.kill('SIGTERM', {
forceKill: false,
forceKillAfter: 50
});

t.true(isRunning(subprocess.pid));
subprocess.kill('SIGKILL');
});

test('execa() with .kill() after it with SIGTERM should kill after 50 ms with SIGKILL', async t => {
const subprocess = execa('node', ['fixtures/no-killable'], {
stdio: ['ipc']
});

await pEvent(subprocess, 'message');

subprocess.kill('SIGTERM', {
forceKillAfter: 50
});

const {signal} = await t.throwsAsync(subprocess);
t.is(signal, 'SIGKILL');
});

test('execa() with .kill() after it with nothing (undefined) should kill after 50 ms with SIGKILL', async t => {
const subprocess = execa('node', ['fixtures/no-killable'], {
stdio: ['ipc']
});

await pEvent(subprocess, 'message');

subprocess.kill();

const {signal} = await t.throwsAsync(subprocess);
t.is(signal, 'SIGKILL');
});
}

test('stripFinalNewline: true', async t => {
const {stdout} = await execa('noop', ['foo']);
t.is(stdout, 'foo');
Expand Down