Remix clone Hacker News

> This isn't actually about removing the promise (completion) listener, but the fact that promises are not cancelable in JS.

You've made an interesting point about promise cancellation but it's ultimately orthogonal to the Github issue I was responding to. The case in question was one in which a memory leak was triggered specifically by racing a long lived promise with another promise — not simply the existence of the promise — but specifically racing that promise against another promise with a shorter lifetime. You shouldn't have to cancel that long lived promise in order to resolve the memory leak. The user who created the issue was creating a promise that resolved whenever the SIGINT signal was received. Why should you have to cancel this promise early in order to tame the memory usage (and only while racing it against another promise)?

As the Node contributor discovered the reason is because semantically `Promise.race` operates similarly to this [1]:

  function race<X, Y>(x: PromiseLike<X>, y: PromiseLike<Y>) {
    return new Promise((resolve, reject) => {
      x.then(resolve, reject)
      y.then(resolve, reject)
    })
  }

Assuming `x` is our non-settling promise, he was able to resolve the memory leak by monkey patching `x` and replacing its then method with a no-op which ignores the resolve and reject listeners: `x.then = () => {};`. Now of course, ignoring the listeners is obviously not ideal, and if there was a native mechanism for removing the resolve and reject listeners `Promise.race` would've used it (perhaps using `y.finally()`) which would have solved the memory leak.

[1] https://github.com/nodejs/node/issues/17469#issuecomment-349...

▲ kaoD 2 days ago | parent | next [-]

> Why should you have to cancel this promise early in order to tame the memory usage (and only while racing it against another promise)?

In the particular case you linked to, the issue is (partially) solved because the promise is short-lived so the `then` makes it live longer, exacerbating the issue. By not then-ing the GC kicks earlier since nothing else holds a reference to its stack frame.

But the underlying issue is lack of cancellation, so if you race a long-lived resource-intensive promise against a short-lived promise, the issue would still be there regardless of listener registration (which admittedly makes the problem worse).

Note that this is still relevant because it means that the problem can kick in in the "middle" of the async function (if any of the inner promises is long) while the `then` problem (which the "middle of the promise" is a special case of "multiple thens", since each await point is isomorphic to calling `then` with the rest of the function).

Without proper cancellation you only solve the particular case if your issue is the latest body of the `then` chain.

(Apologies for the unclear explanation, I'm on mobile and on the vet's waiting room, I'm trying my best.)

▲ throwitaway1123 2 days ago | parent [-]

I don't want to get mired in a theoretical discussion about what promise cancellation would hypothetically look like, and would rather instead look at some concrete code. If you reproduce the memory leak from that original Node Github issue while setting the --max-old-space-size to an extremely low number (to set a hard limit on memory usage) you can empirically observe that the Node process crashes almost instantly with a heap out of memory error:

  #! /usr/bin/env node --max-old-space-size=5
  
  const interruptPromise = new Promise(resolve =>
    process.once('SIGINT', () => resolve('interrupted'))
  )
  
  async function run() {
    while (true) {
      const taskPromise = new Promise(resolve => setImmediate(resolve))
      const result = await Promise.race([taskPromise, interruptPromise])
      if (result === 'interrupted') break
    }
    console.log(`SIGINT`)
  }
  
  run()

If you run that exact same code but replace `Promise.race` with a call to `Unpromise.race`, the program appears to run indefinitely and memory usage appears to plateau. And if you look at the definition of `Unpromise.race`, the author is saying almost exactly the same thing that I've been saying: "Equivalent to Promise.race but eliminates memory leaks from long-lived promises accumulating .then() and .catch() subscribers" [1], which is exactly the same thing that the Node contributor from the original issue was saying, which is also exactly the same thing the Chromium contributor was saying in the Chromium bug report where he writes "This will also grow the reactions list of `x` to 10e5" [2].

[1] https://github.com/cefn/watchable/blob/6a2cd66537c664121671e...

[2] https://issues.chromium.org/issues/42213031#comment5

	▲	kaoD a day ago \| parent [-]
		Just to clarify because the message might have been lost: I'm not saying you're wrong! I'm saying you're right, and... Quoting a comment from the issue you linked: > This is not specific to Promise.race, but for any callback attached a promise that will never be resolved like this: `x = new Promise(() => {}); for (let i = 0; i < 10e5 ; i++) { x.then(() => {}); }` My point is if you do something like this (see below) instead, the same issue is still there and cannot be resolved just by using `Unpromise.race` because the underlying issue is promise cancellation: // Use this in the `race` instead // Will also leak memory even with `Unpromise.race` const interruptPromiseAndLog = () => interruptPromise() .then(() => console.log('SIGINT')) `Unpromise.race` only helps with its internal `then` so it will only help if the promise you're using has no inner `then` or `await` after the non-progressing point. This is not a theoretical issue. This code happens all the time naturally, including in library code that you have no control over. So you have to proxy this promise too... but again this only partially solves the issue because you'd have to promise every single promise that might ever be created, including those you have no control over (in library code) and therefore cannot proxy yourself. And the ergonomics are terrible. If you do this, you have to proxy and propagate unsubscription to both `then`s: `const interruptPromiseAndLog = () => interruptPromise() // How do you unsubscribe this one .then(() => console.log('SIGINT')) // ...even if you can easily proxy this one? .then(() => console.log('REALLY SIGINT'))` Which can easily happen in await points too: `const interruptPromiseAndLog = async () => { console.log('Waiting for SIGINT') // You have to proxy and somehow propagate unsubscription to this one too... how!? await interruptPromise() console.log('SIGINT') }` Since this is just sugar for: `const interruptPromiseAndLog = () => { console.log('Waiting for SIGINT') return interruptPromise() // Needs unsubscription forwarded here .then(() => console.log('SIGINT')) }` Which can quickly get out of hand with multiple await points (i.e. many `then`s). Hence why I say the underlying issue is overall promise cancellation and how you actually have no ownership of promises in JS userspace, only of their completion handles (the event loop is the actual promise owner) which do nothing when going out of scope (only the handle is GC'd but the promise stays alive in the event loop).

▲ GoblinSlayer 2 days ago | parent | prev [-]

For that matter C# has Task.WaitAsync, so waited task continues to the waiter task, and your code subscribes to the waiter task, which unregisters your listener after firing it, so memory leak is limited to the small waiter task that doesn't refer anything after timeout.