Skip to content

Commit

Permalink
timers: cleanup abort listener on awaitable timers
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
jasnell authored and danielleadams committed Nov 9, 2020
1 parent 567f8d8 commit 77e4f19
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 17 deletions.
42 changes: 26 additions & 16 deletions lib/timers/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const {
Promise,
PromisePrototypeFinally,
PromiseReject,
} = primordials;

Expand All @@ -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') {
Expand Down Expand Up @@ -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 = {}) {
Expand Down Expand Up @@ -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 = {
Expand Down
23 changes: 22 additions & 1 deletion test/parallel/test-timers-promisified.js
Original file line number Diff line number Diff line change
@@ -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 */
Expand Down Expand Up @@ -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)), {
Expand Down

0 comments on commit 77e4f19

Please sign in to comment.