From 538ec982a5c51fb342c00bf0c57124f99dfc5887 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= <targos@protonmail.com>
Date: Sun, 16 May 2021 14:41:54 +0200
Subject: [PATCH] timers: cleanup abort listener on awaitable timers

Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: https://github.com/nodejs/node/pull/36006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 lib/internal/timers/promises.js          | 42 +++++++++++++++---------
 test/parallel/test-timers-promisified.js | 23 ++++++++++++-
 2 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/lib/internal/timers/promises.js b/lib/internal/timers/promises.js
index 94c8f40a19db5e..f1912642a8faeb 100644
--- a/lib/internal/timers/promises.js
+++ b/lib/internal/timers/promises.js
@@ -2,6 +2,7 @@
 
 const {
   Promise,
+  PromisePrototypeFinally,
   PromiseReject,
 } = primordials;
 
@@ -26,6 +27,13 @@ const lazyDOMException = hideStackFrames((message, name) => {
   return new DOMException(message, name);
 });
 
+function cancelListenerHandler(clear, reject) {
+  if (!this._destroyed) {
+    clear(this);
+    reject(lazyDOMException('The operation was aborted', 'AbortError'));
+  }
+}
+
 function setTimeout(after, value, options = {}) {
   const args = value !== undefined ? [value] : value;
   if (options == null || typeof options !== 'object') {
@@ -55,20 +63,21 @@ function setTimeout(after, value, options = {}) {
     return PromiseReject(
       lazyDOMException('The operation was aborted', 'AbortError'));
   }
-  return new Promise((resolve, reject) => {
+  let oncancel;
+  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', () => {
-        if (!timeout._destroyed) {
-          // eslint-disable-next-line no-undef
-          clearTimeout(timeout);
-          reject(lazyDOMException('The operation was aborted', 'AbortError'));
-        }
-      }, { once: true });
+      // eslint-disable-next-line no-undef
+      oncancel = cancelListenerHandler.bind(timeout, clearTimeout, reject);
+      signal.addEventListener('abort', oncancel);
     }
   });
+  return oncancel !== undefined ?
+    PromisePrototypeFinally(
+      ret,
+      () => signal.removeEventListener('abort', oncancel)) : ret;
 }
 
 function setImmediate(value, options = {}) {
@@ -99,19 +108,20 @@ function setImmediate(value, options = {}) {
     return PromiseReject(
       lazyDOMException('The operation was aborted', 'AbortError'));
   }
-  return new Promise((resolve, reject) => {
+  let oncancel;
+  const ret = new Promise((resolve, reject) => {
     const immediate = new Immediate(resolve, [value]);
     if (!ref) immediate.unref();
     if (signal) {
-      signal.addEventListener('abort', () => {
-        if (!immediate._destroyed) {
-          // eslint-disable-next-line no-undef
-          clearImmediate(immediate);
-          reject(lazyDOMException('The operation was aborted', 'AbortError'));
-        }
-      }, { once: true });
+      // eslint-disable-next-line no-undef
+      oncancel = cancelListenerHandler.bind(immediate, clearImmediate, reject);
+      signal.addEventListener('abort', oncancel);
     }
   });
+  return oncancel !== undefined ?
+    PromisePrototypeFinally(
+      ret,
+      () => signal.removeEventListener('abort', oncancel)) : ret;
 }
 
 module.exports = {
diff --git a/test/parallel/test-timers-promisified.js b/test/parallel/test-timers-promisified.js
index 1b1e98d628543a..cc399746fbe77b 100644
--- a/test/parallel/test-timers-promisified.js
+++ b/test/parallel/test-timers-promisified.js
@@ -1,10 +1,13 @@
-// Flags: --no-warnings --experimental-abortcontroller
+// Flags: --no-warnings --expose-internals --experimental-abortcontroller
 'use strict';
 const common = require('../common');
 const assert = require('assert');
 const timers = require('timers');
 const { promisify } = require('util');
 
+// TODO(benjamingr) - refactor to use getEventListeners when #35991 lands
+const { NodeEventTarget } = require('internal/event_target');
+
 /* eslint-disable no-restricted-syntax */
 
 const setTimeout = promisify(timers.setTimeout);
@@ -85,6 +88,24 @@ process.on('multipleResolves', common.mustNotCall());
   });
 }
 
+{
+  // Check that timer adding signals does not leak handlers
+  const signal = new NodeEventTarget();
+  signal.aborted = false;
+  setTimeout(0, null, { signal }).finally(common.mustCall(() => {
+    assert.strictEqual(signal.listenerCount('abort'), 0);
+  }));
+}
+
+{
+  // Check that timer adding signals does not leak handlers
+  const signal = new NodeEventTarget();
+  signal.aborted = false;
+  setImmediate(0, { signal }).finally(common.mustCall(() => {
+    assert.strictEqual(signal.listenerCount('abort'), 0);
+  }));
+}
+
 {
   Promise.all(
     [1, '', false, Infinity].map((i) => assert.rejects(setImmediate(10, i)), {