Skip to content

Commit

Permalink
child_process: clean event listener correctly
Browse files Browse the repository at this point in the history
I was working on AbortSignal for spawn and noticed there is a leak in
the current code for AbortSignal support in child_process since it
removes the wrong listener. I used the new signal as argument feature
to make removing the listener easier and added a test.

PR-URL: #36424
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
benjamingr authored and danielleadams committed Jan 12, 2021
1 parent 63091f8 commit 7134d49
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 5 deletions.
9 changes: 4 additions & 5 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -378,14 +378,13 @@ function execFile(file /* , args, options, callback */) {
if (options.signal.aborted) {
process.nextTick(() => kill());
} else {
const childController = new AbortController();
options.signal.addEventListener('abort', () => {
if (!ex) {
if (!ex)
ex = new AbortError();
}
kill();
});
const remove = () => options.signal.removeEventListener('abort', kill);
child.once('close', remove);
}, { signal: childController.signal });
child.once('close', () => childController.abort());
}
}

Expand Down
11 changes: 11 additions & 0 deletions test/parallel/test-child-process-execfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const common = require('../common');
const assert = require('assert');
const execFile = require('child_process').execFile;
const { getEventListeners } = require('events');
const { getSystemErrorName } = require('util');
const fixtures = require('../common/fixtures');

Expand Down Expand Up @@ -68,5 +69,15 @@ const execOpts = { encoding: 'utf8', shell: true };

execFile(process.execPath, [echoFixture, 0], { signal: 'hello' }, callback);
}, { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError' });
}
{
// Verify that the process completing removes the abort listener
const ac = new AbortController();
const { signal } = ac;

const callback = common.mustCall((err) => {
assert.strictEqual(getEventListeners(ac.signal).length, 0);
assert.strictEqual(err, null);
});
execFile(process.execPath, [fixture, 0], { signal }, callback);
}

0 comments on commit 7134d49

Please sign in to comment.