Commit 77e4f19
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>1 parent 567f8d8 commit 77e4f19
2 files changed
Lines changed: 48 additions & 17 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -2,6 +2,7 @@ | |||
| 2 | 2 | ||
| 3 | 3 | const { | |
| 4 | 4 | Promise, | |
| 5 | + PromisePrototypeFinally, | ||
| 5 | 6 | PromiseReject, | |
| 6 | 7 | } = primordials; | |
| 7 | 8 | ||
@@ -24,6 +25,13 @@ const lazyDOMException = hideStackFrames((message, name) => { | |||
| 24 | 25 | return new DOMException(message, name); | |
| 25 | 26 | }); | |
| 26 | 27 | ||
| 28 | + function cancelListenerHandler(clear, reject) { | ||
| 29 | + if (!this._destroyed) { | ||
| 30 | + clear(this); | ||
| 31 | + reject(lazyDOMException('The operation was aborted', 'AbortError')); | ||
| 32 | + } | ||
| 33 | + } | ||
| 34 | + | ||
| 27 | 35 | function setTimeout(after, value, options = {}) { | |
| 28 | 36 | const args = value !== undefined ? [value] : value; | |
| 29 | 37 | if (options == null || typeof options !== 'object') { | |
@@ -58,20 +66,21 @@ function setTimeout(after, value, options = {}) { | |||
| 58 | 66 | return PromiseReject( | |
| 59 | 67 | lazyDOMException('The operation was aborted', 'AbortError')); | |
| 60 | 68 | } | |
| 61 | - return new Promise((resolve, reject) => { | ||
| 69 | + let oncancel; | ||
| 70 | + const ret = new Promise((resolve, reject) => { | ||
| 62 | 71 | const timeout = new Timeout(resolve, after, args, false, true); | |
| 63 | 72 | if (!ref) timeout.unref(); | |
| 64 | 73 | insert(timeout, timeout._idleTimeout); | |
| 65 | 74 | if (signal) { | |
| 66 | - signal.addEventListener('abort', () => { | ||
| 67 | - if (!timeout._destroyed) { | ||
| 68 | - // eslint-disable-next-line no-undef | ||
| 69 | - clearTimeout(timeout); | ||
| 70 | - reject(lazyDOMException('The operation was aborted', 'AbortError')); | ||
| 71 | - } | ||
| 72 | - }, { once: true }); | ||
| 75 | + // eslint-disable-next-line no-undef | ||
| 76 | + oncancel = cancelListenerHandler.bind(timeout, clearTimeout, reject); | ||
| 77 | + signal.addEventListener('abort', oncancel); | ||
| 73 | 78 | } | |
| 74 | 79 | }); | |
| 80 | + return oncancel !== undefined ? | ||
| 81 | + PromisePrototypeFinally( | ||
| 82 | + ret, | ||
| 83 | + () => signal.removeEventListener('abort', oncancel)) : ret; | ||
| 75 | 84 | } | |
| 76 | 85 | ||
| 77 | 86 | function setImmediate(value, options = {}) { | |
@@ -107,19 +116,20 @@ function setImmediate(value, options = {}) { | |||
| 107 | 116 | return PromiseReject( | |
| 108 | 117 | lazyDOMException('The operation was aborted', 'AbortError')); | |
| 109 | 118 | } | |
| 110 | - return new Promise((resolve, reject) => { | ||
| 119 | + let oncancel; | ||
| 120 | + const ret = new Promise((resolve, reject) => { | ||
| 111 | 121 | const immediate = new Immediate(resolve, [value]); | |
| 112 | 122 | if (!ref) immediate.unref(); | |
| 113 | 123 | if (signal) { | |
| 114 | - signal.addEventListener('abort', () => { | ||
| 115 | - if (!immediate._destroyed) { | ||
| 116 | - // eslint-disable-next-line no-undef | ||
| 117 | - clearImmediate(immediate); | ||
| 118 | - reject(lazyDOMException('The operation was aborted', 'AbortError')); | ||
| 119 | - } | ||
| 120 | - }, { once: true }); | ||
| 124 | + // eslint-disable-next-line no-undef | ||
| 125 | + oncancel = cancelListenerHandler.bind(immediate, clearImmediate, reject); | ||
| 126 | + signal.addEventListener('abort', oncancel); | ||
| 121 | 127 | } | |
| 122 | 128 | }); | |
| 129 | + return oncancel !== undefined ? | ||
| 130 | + PromisePrototypeFinally( | ||
| 131 | + ret, | ||
| 132 | + () => signal.removeEventListener('abort', oncancel)) : ret; | ||
| 123 | 133 | } | |
| 124 | 134 | ||
| 125 | 135 | module.exports = { | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1,11 +1,14 @@ | |||
| 1 | - // Flags: --no-warnings | ||
| 1 | + // Flags: --no-warnings --expose-internals | ||
| 2 | 2 | 'use strict'; | |
| 3 | 3 | const common = require('../common'); | |
| 4 | 4 | const assert = require('assert'); | |
| 5 | 5 | const timers = require('timers'); | |
| 6 | 6 | const { promisify } = require('util'); | |
| 7 | 7 | const child_process = require('child_process'); | |
| 8 | 8 | ||
| 9 | + // TODO(benjamingr) - refactor to use getEventListeners when #35991 lands | ||
| 10 | + const { NodeEventTarget } = require('internal/event_target'); | ||
| 11 | + | ||
| 9 | 12 | const timerPromises = require('timers/promises'); | |
| 10 | 13 | ||
| 11 | 14 | /* eslint-disable no-restricted-syntax */ | |
@@ -92,6 +95,24 @@ process.on('multipleResolves', common.mustNotCall()); | |||
| 92 | 95 | }); | |
| 93 | 96 | } | |
| 94 | 97 | ||
| 98 | + { | ||
| 99 | + // Check that timer adding signals does not leak handlers | ||
| 100 | + const signal = new NodeEventTarget(); | ||
| 101 | + signal.aborted = false; | ||
| 102 | + setTimeout(0, null, { signal }).finally(common.mustCall(() => { | ||
| 103 | + assert.strictEqual(signal.listenerCount('abort'), 0); | ||
| 104 | + })); | ||
| 105 | + } | ||
| 106 | + | ||
| 107 | + { | ||
| 108 | + // Check that timer adding signals does not leak handlers | ||
| 109 | + const signal = new NodeEventTarget(); | ||
| 110 | + signal.aborted = false; | ||
| 111 | + setImmediate(0, { signal }).finally(common.mustCall(() => { | ||
| 112 | + assert.strictEqual(signal.listenerCount('abort'), 0); | ||
| 113 | + })); | ||
| 114 | + } | ||
| 115 | + | ||
| 95 | 116 | { | |
| 96 | 117 | Promise.all( | |
| 97 | 118 | [1, '', false, Infinity].map((i) => assert.rejects(setImmediate(10, i)), { | |
0 commit comments