timers: cleanup abort listener on awaitable timers

jasnell · danielleadams · commit 77e4f1970103 · 2020-11-09T13:35:04.000-05:00
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>
diff --git a/lib/timers/promises.js b/lib/timers/promises.js
@@ -2,6 +2,7 @@
 
 const {
   Promise,
+  PromisePrototypeFinally,
   PromiseReject,
 } = primordials;
 
@@ -24,6 +25,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') {
@@ -58,20 +66,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 = {}) {
@@ -107,19 +116,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,11 +1,14 @@
-// Flags: --no-warnings
+// Flags: --no-warnings --expose-internals
 'use strict';
 const common = require('../common');
 const assert = require('assert');
 const timers = require('timers');
 const { promisify } = require('util');
 const child_process = require('child_process');
 
+// TODO(benjamingr) - refactor to use getEventListeners when #35991 lands
+const { NodeEventTarget } = require('internal/event_target');
+
 const timerPromises = require('timers/promises');
 
 /* eslint-disable no-restricted-syntax */
@@ -92,6 +95,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)), {
