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>

Author: targos

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 = {

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)), {