potential for memory leakage

[skeggse]

Any time application code uses delay or adds a cancellation handler via context.cancelled.then(...), that promise handler never gets cleaned up independent of the context. For long-running contexts, this can be problematic, as the embedded promise will simply accumulate cancellation handlers. Oftentimes, these handlers become unnecessary (and useless) after some subset of the time the context is alive - I'm wondering if there's an alternative approach that would allow the addition and removal of cancellation handlers (which would likely require a different promise or promiselike implementation).

For example, this code will leak memory despite it not being obvious.

import context from '@kriskowal/context';

// this runs "forever" (until the iterable encounters an upstream socket error, say)
async function consumeChangeStream(context, iterable) {
  for await (const entry of iterable) {
    // Make a request for each entry in the change stream. Per the example in
    // the README, this would use context.cancelled.then to register a handler
    // that invokes abort on the abortController. Since this context is never
    // directly halted until the process receives a SIGINT, the context's
    // cancelled promise will just accumulate handlers.
    await fetchWithContext(context, entry.path, { method: 'POST' });

    // Rate-limit the consumption of the change-stream (and maybe even apply
    // extra backpressure!)
    await context.delay(1000);
  }
}

const { cancel, context: procContext } = context.withCancel();
process.on('SIGINT', () => cancel(new Error('process exiting')));

consumeChangeStream(procContext, getChangeStream()).catch((err) => {
  process.nextTick(() => {
    throw err;
  });
});

EDIT: ah, and it looks like this applies to withCancel as well

[bminer]

I agree. This is an issue. In Go, context cancellation is checked by reading from a channel returned by Done(). No memory leaks in Go. Here's my solution for JavaScript:

Instead of this leaky solution:

context.cancelled.then(function onCancel() {
    // cancel something
})
async doSomethingCancelable()

Here's an alternative that requires no changes to this library:

const [cancelled, result] = await Promise.race([
    context.cancelled,
    doSomethingCancelable(),
])
if(cancelled) {
    // cancel something (note: `cancelled` is an Error)
}

Things to note:

• context.cancelled is never rejected.
• context.cancelled either resolves to a cancellation error or never resolves. This seems strange at first glance, but context.cancelled cannot simply reject. If it did, it is possible that the rejection would be unhandled, which would be bad: unhandled Promise rejections are handled much like uncaught exceptions (for good reason).
• If context is cancelled after doSomethingCancelable() resolves / rejects, the cancellation signal is lost (i.e. no one is listening; thus, no leaks)

[bminer]

If the above code works, I think documentation changes are in order. Thoughts?

[cspotcode]

If I understand correctly, this relies on Promise.race and context.cancelled both being native Promise code?

It seems like a promise polyfill could not avoid a memory leak, since Promise.race() would need to pass a callback to context.cancelled.then().

Not that this is a bad thing. I don't use polyfills.

[cspotcode]

I thought about some API ideas to achieve "disposal" of cancellation listeners to avoid memory leaks.

A

Add helper method to register onCancelCallback, run asyncWorkFunction, and unregister the callback when the work function settles.

context.doWithCancellationHandler(onCancelCallback, asyncWorkFunction);

B

Context implements Disposable interface. Can create a child context and .dispose() it when done, which clears all cancelled listeners of the child context.

context.cancelled.then(); context.dispose();

C

Add .onCancelled which is like .cancelled.then() but returns a Disposable

const disposable = context.onCancelled(onCancelCallback); disposable.dispose();