From 6c08c9de4a1285b4ffeba8d73575c61df82da53f Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Mon, 7 Dec 2020 12:54:50 +0200 Subject: [PATCH] child_process: clean event listener correctly 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: https://github.com/nodejs/node/pull/36424 Backport-PR-URL: https://github.com/nodejs/node/pull/38386 Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott --- lib/child_process.js | 9 ++++----- test/parallel/test-child-process-execfile.js | 11 +++++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 1b645e43c8151b..28290f65a0fb2d 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -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()); } } diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 6012072eb297d1..ca430d17cfe1bb 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -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'); @@ -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); }