How to elegantly manage AbortSignal event listeners when implementing abortable APIs?

Question

Consider this simple example, probably a function you wrote a couple of times, but now abortable:

/**
 * 
 * @param {number} delay 
 * @param {AbortSignal} [abortSignal]
 * @returns {Promise<void>}
 */
export default function timeoutPromise(delay, abortSignal) {
    return new Promise((resolve, reject) => {
        if(abortSignal) {
            abortSignal.throwIfAborted();
        }

        const timeout = setTimeout(() => {
            resolve();
        }, delay);

        abortSignal.addEventListener("abort", () => {
            clearTimeout(timeout);
            reject(new Error("Aborted"));
        });
    });
}

The obvious issue is that this will not clear the eventListener if the timeout succeeds normally. It can be done, but it is quite ugly:

/**
 * 
 * @param {number} delay 
 * @param {AbortSignal} [abortSignal]
 * @returns {Promise<void>}
 */
export default function timeoutPromise(delay, abortSignal) {
    return new Promise((resolve, reject) => {
        // note: changed to reject() to get consistent behavior regardless of the signal state
        if(abortSignal && abortSignal.aborted) {
            reject(new Error("timeoutPromise aborted"));
        }
        let timeout = null;
        function abortHandler() {
            clearTimeout(timeout);
            reject(new Error("timeoutPromise aborted"))
        }
        timeout = setTimeout(() => {
            if(abortSignal) {
                abortSignal.removeEventListener("abort", abortHandler);
            }
            resolve();
        }, delay);

        if(abortSignal) {
            abortSignal.addEventListener("abort", abortHandler, {once: true});
        }
    });
}

That's... a lot of code for such a simple thing. Am I doing this right or is there a better way?

Bigeye

a lot of code written slightly differently, it's effectively two extra lines of code – Analisaanalise

Why are you no longer using abortSignal.throwIfAborted();? And what's with the if (abortSignal) in the first place? – Recovery

Why do you want to remove the event listener? If nobody holds any reference to the AbortSignal it will be garbage collected along with it. – Fcc

Also might be noted that for this exact case of an abortable timeout, there is one (only in Chrome currently): scheduler.postTask(cb, { delay, signal }) and it even returns a Promise. – Fcc

@Fcc because the abort signal often outlives the timeout by far – Recovery

@Recovery and? Having the event attached to it adds very little overhead compared to the whole AbortController. – Fcc

@Fcc It's still a memory leak. The abort event handler closes over timeout, reject, and - depending on the promise implementation - even over the promise and its value. You don't want a long-running task with many small delays to accumulate many handlers on its abort signal - typically only the last one is actually able to abort the task. – Recovery

@Fcc This is just a simple example. In general, keeping the listener could capture arbitrary amount of variables with the callback. Also, leaking memory just because you think the leak is small is really not a good approach and not something you'd expect in an "elegant" solution. – Ripley

@Recovery I removed throwIfAborted because it leads to inconsistent behavior. If the signal is aborted, it throws when creating the promise, if the signal were aboreted just a moment later, it would throw only on await. This is relevant when you're awaiting promises later and not the instant you create them, which is needed to execute tasks concurrently. The if(abortSignal) is there because the signal is intended to be optional. – Ripley

@TomášZato "it throws when creating the promise" - no, signal.throwIfAborted() in the executor callback is exactly equivalent to if (signal.aborted) reject(signal.reason). "when you're awaiting promises later and not the instant you create them, which is needed to execute tasks concurrently" - no, it wouldn't make a difference. And you should just never do that anyway! Always use Promise.all for concurrent tasks. – Recovery

@Recovery If you await Promise.all then the total time for your work is the time to resolve the slowest promise plus the sum of time to handle the results. If you use Promise.race to handle responses as they come, you can handle responses in parallel to waiting for the promises, but in that case you need to keep the unresolved promises somewhere. Just recently, I heard colleague talking about a bug when someone blindly followed the Promise.all rule, causing significant bottleneck in the system. – Ripley

@TomášZato You still can handle results as they come with Promise.all, just put the handling code right where you make each request (Promise.all(vals.map(async v => { const response = await request(v); return handle(response); }))). No need for Promise.race or keeping unhandled promises around for some time. – Recovery

nice and educational, both ... the question and the answer. – Siren

@Recovery Ok, that's a good point, you could do that. I guess I was thinking more of the specific example that I am working on, where the number of promises is way to large, so I start some, use Promise.race and replace the completed ones with new ones. That's probably not something typical webdev needs to worry about. I am talking millions of tasks, doing max N in parallel. – Ripley

@TomášZato Sure, there's established patterns to handle that, none of them requiring unhandled promises left dangling – Recovery

Answer 1

You can use optional chaining for all the method calls on the AbortSignal and it becomes more straightforward:

function delay(ms, signal) {
  return new Promise((resolve, reject) => {
    function done() {
      resolve();
      signal?.removeEventListener("abort", stop);
    }
    function stop() {
      reject(this.reason);
      clearTimeout(handle);
    }
    signal?.throwIfAborted();
    const handle = setTimeout(done, ms);
    signal?.addEventListener("abort", stop);
  });
}

^{(from my answer to How to cancel JavaScript sleep?)}

Or do only a single check for the signal existence:

function delay(ms, signal) {
  return new Promise((resolve, reject) => {
    if (!signal) {
      setTimeout(resolve, ms);
      return;
    }

    function done() {
      resolve();
      signal.removeEventListener("abort", stop);
    }
    function stop() {
      reject(this.reason);
      clearTimeout(handle);
    }
    signal.throwIfAborted();
    const handle = setTimeout(done, ms);
    signal.addEventListener("abort", stop);
  });
}

which you can of course golf further:

function delay(ms, signal) {
  return new Promise((resolve, reject) => {
    if (!signal) return setTimeout(resolve, ms);
    signal.throwIfAborted();
    const handle = setTimeout(() => {
      resolve();
      signal.removeEventListener("abort", stop);
    }, ms);
    const stop = () => {
      reject(signal.reason);
      clearTimeout(handle);
    };
    signal.addEventListener("abort", stop);
  });
}

Recovery

Answer 2

The addEventListener() method accepts an optional object of options as its third argument. If you set the signal property, you can remove all event listeners.

/**
 * 
 * @param {number} delay 
 * @param {AbortSignal} [abortSignal]
 * @returns {Promise<void>}
 */
export default function timeoutPromise(delay, abortSignal) {
    return new Promise((resolve, reject) => {
        if(abortSignal) {
            abortSignal.throwIfAborted();
        }

        const clearController = new AbortController();

        const timeout = setTimeout(() => {
            clearController.abort();
            resolve();
        }, delay);

        abortSignal.addEventListener("abort", () => {
            clearController.abort();
            clearTimeout(timeout);
            reject(new Error("Aborted"));
        }, {
            signal: clearController.signal
        });
    });
}

Kamala

Answer 3

I understand the interest in removing the event listener but I think there is a more elegant way to do this using AbortController internally.

function delay(ms, signal) {
  const clonedSignal = AbortSignal.any(signal ? [signal] : []);
  let timer;
  return new Promise((resolve, reject) => {
    clonedSignal.throwIfAborted();
    clonedSignal.onabort = reject;
    timer = setTimeout(resolve, ms);
  }).finally(() => clearTimeout(timer));
}

Woodsman

What's the point of having the cleanup controller? Also did you mean to use cleanup.signal in the AbortSignal.any call? – Recovery

Using finally has the drawback of running clearTimeout even if the timeout already completed – Recovery

Good points. Edited. no need for the cleanup controller. But clearTimeout on an exhausted timeout is a no-op last I checked. – Woodsman