Skip to content

Commit

Permalink
raceWithSignal method instead of Promise.race
Browse files Browse the repository at this point in the history
  • Loading branch information
phryneas authored and markerikson committed Jan 28, 2023
1 parent ae59d5f commit 54bbb32
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 23 deletions.
13 changes: 8 additions & 5 deletions packages/toolkit/src/listenerMiddleware/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ import {
} from './exceptions'
import {
runTask,
promisifyAbortSignal,
validateActive,
createPause,
createDelay,
raceWithSignal,
} from './task'
export { TaskAbortError } from './exceptions'
export type {
Expand Down Expand Up @@ -138,9 +138,9 @@ const createTakePattern = <S>(
// Placeholder unsubscribe function until the listener is added
let unsubscribe: UnsubscribeListener = () => {}

const tuplePromise = new Promise<[AnyAction, S, S]>((resolve) => {
const tuplePromise = new Promise<[AnyAction, S, S]>((resolve, reject) => {
// Inside the Promise, we synchronously add the listener.
unsubscribe = startListening({
let stopListening = startListening({
predicate: predicate as any,
effect: (action, listenerApi): void => {
// One-shot listener that cleans up as soon as the predicate passes
Expand All @@ -153,10 +153,13 @@ const createTakePattern = <S>(
])
},
})
unsubscribe = () => {
stopListening()
reject()
}
})

const promises: (Promise<null> | Promise<[AnyAction, S, S]>)[] = [
promisifyAbortSignal(signal),
tuplePromise,
]

Expand All @@ -167,7 +170,7 @@ const createTakePattern = <S>(
}

try {
const output = await Promise.race(promises)
const output = await raceWithSignal(signal, Promise.race(promises))

validateActive(signal)
return output
Expand Down
41 changes: 23 additions & 18 deletions packages/toolkit/src/listenerMiddleware/task.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { TaskAbortError } from './exceptions'
import type { AbortSignalWithReason, TaskResult } from './types'
import { addAbortSignalListener, catchRejection } from './utils'
import { addAbortSignalListener, catchRejection, noop } from './utils'

/**
* Synchronously raises {@link TaskAbortError} if the task tied to the input `signal` has been cancelled.
Expand All @@ -15,24 +15,29 @@ export const validateActive = (signal: AbortSignal): void => {
}

/**
* Returns a promise that will reject {@link TaskAbortError} if the task is cancelled.
* @param signal
* @returns
* Generates a race between the promise(s) and the AbortSignal
* This avoids `Promise.race()`-related memory leaks:
* https://github.com/nodejs/node/issues/17469#issuecomment-349794909
*/
export const promisifyAbortSignal = (
signal: AbortSignalWithReason<string>
): Promise<never> => {
return catchRejection(
new Promise<never>((_, reject) => {
const notifyRejection = () => reject(new TaskAbortError(signal.reason))
export function raceWithSignal<T>(
signal: AbortSignalWithReason<string>,
promise: Promise<T>
): Promise<T> {
let cleanup = noop
return new Promise<T>((resolve, reject) => {
const notifyRejection = () => reject(new TaskAbortError(signal.reason))

if (signal.aborted) {
notifyRejection()
return
}

if (signal.aborted) {
notifyRejection()
} else {
addAbortSignalListener(signal, notifyRejection)
}
})
)
cleanup = addAbortSignalListener(signal, notifyRejection)
promise.finally(() => cleanup()).then(resolve, reject)
}).finally(() => {
// after this point, replace `cleanup` with a noop, so there is no reference to `signal` any more
cleanup = noop
})
}

/**
Expand Down Expand Up @@ -73,7 +78,7 @@ export const runTask = async <T>(
export const createPause = <T>(signal: AbortSignal) => {
return (promise: Promise<T>): Promise<T> => {
return catchRejection(
Promise.race([promisifyAbortSignal(signal), promise]).then((output) => {
raceWithSignal(signal, promise).then((output) => {
validateActive(signal)
return output
})
Expand Down
1 change: 1 addition & 0 deletions packages/toolkit/src/listenerMiddleware/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const addAbortSignalListener = (
callback: (evt: Event) => void
) => {
abortSignal.addEventListener('abort', callback, { once: true })
return () => abortSignal.removeEventListener('abort', callback)
}

/**
Expand Down

0 comments on commit 54bbb32

Please sign in to comment.