Skip to content

Commit

Permalink
Fix memory leak in buildCancellablePromise (#13)
Browse files Browse the repository at this point in the history
Context:
The `buildCancellablePromise` helper function provides a `capture` hook
to register all promises spawned in the `innerFunc`. The hook stores
references to all these promises in an array.  When the constructed
promise is eventually cancelled, every registered promise is cancelled
as well. This mechanism ensures correct termination of the `innerFunc`
and all cancellable promises that it spawned (and registered).

Memory leak analysis:
The issue is that the array keeps references also to promises that were
already resolved or rejected. This prevents garbage collection of
promises that are of no use anymore and would be otherwise
garbage-collected, were they not stuck in the helper array.

For tasks that repeatedly spawn and await a promise, the array
of captured promises grows indefinitely, as resolved promises
are never removed from that array.

Think about this example:
```
buildCancellablePromise( async C => {
    let counter = 0
    do {
        console.log({counter})
        counter++
        await C(CancellablePromise.delay(0))
        // the promise is captured to the `capturedPromises` array in every iteration
        // which causes the array to grow indefinitely
    } while (true)
})
```

Although this might seem as an artificial example, it sometimes has its
use in practice (think about indefinite task that process input data in
a loop).

The array of registered promises can be garbage-collected only when the
`CancellablePromise` returned from the `buildCancellablePromise` is
itself fitted for garbage-collection.

The top-level CancellablePromise that was created with the
`buildCancellablePromise` helper function transitionally keeps a
reference to all "nested" cancellable promises (pending, resolved and
rejected), which prevents garbage collection of the whole promise tree
until the top-level CancellablePromise is no longer referenced.

Fix:
We fix the memory leak by replacing the `capturedPromises` Array with a
Set. When captured promise is resolved or rejected, it is removed from
the Set data structure, which allows for it being garbage-collected.

Risks:
The cancellation of captured promises was performed in the order how
they were captured, which reflects the promise spawning order.

Replacing the Array with a Set removes this implicit order for
cancellation, and we can no longer guarantee the cancellation
order. This issue should be assessed, and if it turns out to be a
problem, we propose to keep the Array structure and remove the elements
from the array as the captured promises resolve/reject.
  • Loading branch information
ondrej-stanek-ozobot authored Jan 24, 2025
1 parent 7f91680 commit b6835f1
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,11 @@ export type CaptureCancellablePromise = <P extends CancellablePromise<unknown>>(
export function buildCancellablePromise<T>(
innerFunc: (capture: CaptureCancellablePromise) => PromiseLike<T>
): CancellablePromise<T> {
const capturedPromises: CancellablePromise<unknown>[] = [];
const capturedPromises = new Set<CancellablePromise<unknown>>();

const capture: CaptureCancellablePromise = (promise) => {
capturedPromises.push(promise);
capturedPromises.add(promise);
promise.finally(() => capturedPromises.delete(promise)).catch(() => {});
return promise;
};

Expand Down

0 comments on commit b6835f1

Please sign in to comment.