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
Backport-PR-URL: #38386
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
  • Loading branch information
benjamingr authored and targos committed Apr 30, 2021
1 parent dc63ca6 commit 6c08c9d
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 @@ -365,14 +365,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 @@ -4,6 +4,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 @@ -69,5 +70,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 6c08c9d

Please sign in to comment.