observablehq / stdlib

The Observable standard library.
https://observablehq.com/@observablehq/standard-library
ISC License
967 stars 83 forks source link

never promise, mem leak? #12

Closed trusktr closed 6 years ago

trusktr commented 6 years ago

https://github.com/observablehq/notebook-stdlib/blob/5ca144d6c6a5ac4061e5b3da9bca0cab02cbd48d/src/promises/never.js#L3

Does each use of this leak memory? If you .then() one of these, how do you cancel it?

mbostock commented 6 years ago

Yes, this leaks, since the promise is never resolved or rejected (or cancelled). In typically usage within Observable, Promises.never is used only within a try-finally block to cleanup a generator:

try {
  yield Promises.never;
} finally {
  cleanup();
}

The runtime will retain a closure to try to display the revolved value of the promise (indefinitely), so there is a small leak.

I see a couple things we could do to address the leak.

First, the runtime could special-case when a cell returns or yields Promises.never, and never attempt to display it (since the promise will never resolve). However, it isn’t especially useful to special-case this result, as any other cell that references a cell that yields or returns Promises.never will also never resolve and thus the runtime will retain closures/promises for it indefinitely too.

Second, we could replace Promises.never with some special built-in promise that is cell-specific and which resolves only when the cell is disposed by the runtime. That is, when the cell is re-run or when another cell it references changes value. It will take some runtime support for this as the meaning of this promise will be cell-local, despite appearing to be a global built-in. It might look something like this:

try {
  await termination;
} finally {
  cleanup();
}