events: remove listener after timer is finished

[benjamingr]

Fixes the memory leak bug described in #35990 (comment)

It is very important to me to make the point that I believe we need to do #35990 (comment) if we want users not to run into this footfun.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/34122/

[mscdex]

I think this should target timers: in the commit message since it's changing timers code?

[jasnell]

This also needs to apply the same fix to the setImmediate() variant.. Here's the patch I was getting ready to open a PR with:

From 5d7b661306188dfc2c1ca282fcbddbe3ac81cca6 Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Fri, 6 Nov 2020 07:07:43 -0800
Subject: [PATCH] timers: cleanup abort listener on awaitable timers

Signed-off-by: James M Snell <jasnell@gmail.com>
---
 lib/timers/promises.js | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/lib/timers/promises.js b/lib/timers/promises.js
index ef1e6437d4..da12eb8cd2 100644
--- a/lib/timers/promises.js
+++ b/lib/timers/promises.js
@@ -58,20 +58,24 @@ function setTimeout(after, value, options = {}) {
     return PromiseReject(
       lazyDOMException('The operation was aborted', 'AbortError'));
   }
-  return new Promise((resolve, reject) => {
+  let cancelListener = undefined;
+  const ret = new Promise((resolve, reject) => {
     const timeout = new Timeout(resolve, after, args, false, true);
     if (!ref) timeout.unref();
     insert(timeout, timeout._idleTimeout);
     if (signal) {
-      signal.addEventListener('abort', () => {
+      cancelListener = () => {
         if (!timeout._destroyed) {
           // eslint-disable-next-line no-undef
           clearTimeout(timeout);
           reject(lazyDOMException('The operation was aborted', 'AbortError'));
         }
-      }, { once: true });
+      };
+      signal.addEventListener('abort', cancelListener);
     }
   });
+  return cancelListener !== undefined ?
+      ret.finally(() => signal.removeEventListener(cancelListener)) : ret;
 }

 function setImmediate(value, options = {}) {
@@ -107,19 +111,23 @@ function setImmediate(value, options = {}) {
     return PromiseReject(
       lazyDOMException('The operation was aborted', 'AbortError'));
   }
-  return new Promise((resolve, reject) => {
+  let cancelListener = undefined;
+  const ret = new Promise((resolve, reject) => {
     const immediate = new Immediate(resolve, [value]);
     if (!ref) immediate.unref();
     if (signal) {
-      signal.addEventListener('abort', () => {
+      cancelListener = () => {
         if (!immediate._destroyed) {
           // eslint-disable-next-line no-undef
           clearImmediate(immediate);
           reject(lazyDOMException('The operation was aborted', 'AbortError'));
         }
-      }, { once: true });
+      };
+      signal.addEventListener('abort', cancelListener);
     }
   });
+  return cancelListener !== undefined ?
+      ret.finally(() => signal.removeEventListener(cancelListener)) : ret;
 }

 module.exports = {
--
2.17.1

Slightly different approach using finally that I think simplifies things overall.

[benjamingr]

@jasnell this is a small fix and I don't want to create needless process - the fix you're proposing looks fine.

Feel free to either push another commit directly to this branch, open a new one or whatever you prefer (probably take the test from here? I checked and it fails on master).

[mcollina]

I would prefer to not allocate a closure here. This adds overhead.

[benjamingr]

I think you have to capture the handler in a closure since you need a reference to it to remove it.

[benjamingr]

Superseded by #36006