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: #36006 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs · May 16, 2021 · 538ec98 · 538ec98
1 parent 1687197
commit 538ec98
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 17 deletions.
diff --git 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
@@ -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)), {