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
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>

2 files changed, 15 insertions, 5 deletions

diff --git a/lib/child_process.js b/lib/child_process.js
index ae5a5f6587..29a6450210 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -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());
     }
   }
 
diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js
index a6345a7e5f..aec80b4e2b 100644
--- a/test/parallel/test-child-process-execfile.js
+++ b/test/parallel/test-child-process-execfile.js
@@ -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');
 
@@ -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);
 }