10

I'm in a situation where I need execute async functions in "parallel", and continue program execution with the best result. Thus I wrote something like this :

var p = [];

for (var i = 0; i < 10; ++i) (function (index) {
  p.push(new Promise(function (resolve, reject) {
    setTimeout(function () {
      var success = Math.random() > 0.7;

      console.log("Resolving", index, "as", success ? "success" : "failure");

      success && resolve(index);
    }, Math.random() * 5000 + 200);
  }));
})(i);

Promise.race(p).then(function (res) {
  console.log("FOUND", res);
}).catch(function (err) {
  console.log("ERROR", err);
});

Now, I'm wondering if this is good practice when working with promises? Is not resolving or rejecting them more often then anything create memory leaks? Are they all eventually GC'ed every time?

Yanick Rochon
  • 51,409
  • 25
  • 133
  • 214
  • Possible duplicate of [Does never resolved promise cause memory leak?](https://stackoverflow.com/q/20068467/1048572) – Bergi Oct 25 '20 at 23:22

1 Answers1

3

The only reason this causes a memory leak is because p is a global variable. Set p = null; at the end, or avoid using a global variable:

var console = { log: function(msg) { div.innerHTML += msg + "<br>"; }};

Promise.race(new Array(10).fill(0).map(function(entry, index) {
  return (function(index) {
    return new Promise(function(resolve) {
      setTimeout(function() {
        var success = Math.random() > 0.7;
        console.log((success? "R":"Not r") + "esolving "+ index +".");
        success && resolve(index);
      }, Math.random() * 5000 + 200);
    });
  })(index);
})).then(function (res) {
    console.log("FOUND: " + res);
}).catch(function (err) {
    console.log("ERROR: " + err);
});
<div id="div"></div>

Promises are garbage and cycle collected when nothing holds on to them, which is when your JS and the browser (here setTimeout) have no references to them anymore.

As soon as one entry in p succeeds or something fails, whichever is sooner, and setTimeout will have let go of everything after 5.2 seconds. JavaScript will then happily garbage collect the promises whether they've been resolved, rejected, or neither. No harm.

The only thing you want to avoid is garbage collecting rejected promises, as that is likely to trigger a browser warning because it is indicative of a web programming bug.

Of course, there's a 30% chance none of them resolve, in which case this will leak (until you close the tab).

Is this good design?

I think it depends on what the functions are doing. If they are lightweight, then I don't see a problem. If they are doing heavy computations or have side-effects, then they hopefully come with some API to cancel the operations, to help save on resources.

narduw
  • 40
  • 1
  • 5
jib
  • 40,579
  • 17
  • 100
  • 158
  • Promise.race will not garbage collect the remaining promises and running Promise.race repeatedly with a promise which does not resolve is a known memory leak. See https://github.com/nodejs/node/issues/17469 – brainkim Aug 31 '20 at 21:40
  • @brainkim That's one implementation, and it sounds like they a [agree it's a bug](https://github.com/nodejs/node/issues/17469#issuecomment-535962603) in V8. – jib Aug 31 '20 at 22:39
  • If you check the related bug on Chromium you’ll see no such agreement. https://bugs.chromium.org/p/v8/issues/detail?id=9858#c4 – brainkim Sep 01 '20 at 01:29
  • @brainkim Thanks for the link, that's interesting. But the OP isn't asking about never-resolved promises, but whether using `Promise.race` will somehow prevent the other promises from "eventually" being GC'ed. – jib Sep 01 '20 at 03:25
  • I came across this answer because the values of Promise.race not being GC’ed when one is pending is pretty much exactly what I’m noticing. You can reproduce this issue in node.js with the following code snippet: https://gist.github.com/brainkim/6fbcdf22db7f59587a84a79f520e9b11. You’re unlikely to want to create 10000 length strings of random characters at the speed of setTimeout, but it shows the linear growth and reaches process heap limit quickly. – brainkim Sep 01 '20 at 05:15
  • Also the OP’s ode snippet has never-resolved promises in it. – brainkim Sep 01 '20 at 06:05
  • 1
    @brainkim you're right. It's been a while since I looked at this. I may have misread `success && resolve(index)` when I answered. Feel free to add a better answer. – jib Sep 01 '20 at 19:33
  • "*it looks like non-resolved promises are not GC'ed in the V8 JS engine*" is the wrong conclusion. Non-resolved promises are garbage collected in exactly the same way as resolved promises - when nothing holds onto them any more. This reference to the promise object can be a property or variable like `p`, or it can be a resolver function. In the particular code, the resolver function(s) of the `Promise.race(p)` promise are put in the reaction lists of the promises in the `p` array. – Bergi Sep 01 '20 at 20:06
  • "*`Promise.race` lets go of `p` as soon as one entry in `p` succeeds or something fails*" - no. The `Promise.race(p)` never held onto `p` (apart from its iteration during the synchronous execution). It's the other way round: the promises in `p` are holding onto the `Promise.race(p)` promise (until that gets settled, ideally). And what's holding these promises is the `p` array and the `setTimeout` closures that might resolve the promises. As soon as all the timeouts are done and `p` is garbage-collected, nothing holds onto the promises any more. – Bergi Sep 01 '20 at 20:10
  • I mean, personally, I think this is unacceptable behavior for Promise.race and wish the behavior was as desribed in the above answer. – brainkim Sep 01 '20 at 20:14
  • @brainkim "*the values of Promise.race not being GC’ed when one is pending*" - that's not what [issue 9859](https://bugs.chromium.org/p/v8/issues/detail?id=9858) is about as far as I can tell. There's two possible sources of leaks if you execute `Promise.race(neverPromise, someValue)` over and over: 1) the resolver function that `Promise.race` puts into the reaction list of `neverPromise` not being cleared, and `neverPromise` having a growing list of functions that it will never call. … – Bergi Sep 01 '20 at 20:19
  • …cont'd: (This should be fixed by using `neverPromise`'s resolver function - that has already been garbage-collected - as an ephemeron). 2) the resolverFunction that could have resolved the `Promise.race` still holding onto that promise (and by extension, the `someValue`) even if it already has been resolved, where the resolver function is a no-op now. (This should be fixed by nullifying the reference to the promise in the resolverFunction's closure scope when the promise is resolved). Fixing 1 is complicated, fixing 2 is easy (if it happens at all) – Bergi Sep 01 '20 at 20:19
  • If that was all the leak was, it would be more gradual and difficult to detect. The problem I’m having right now is that somehow the reaction created by Promise.race for a forever pending promise somehow retains the other values passed to race, such that what’s passed as someValue directly affects the severity of the leak. See this gist and observe the increase in memory when you pass smaller or larger random strings https://gist.github.com/brainkim/6fbcdf22db7f59587a84a79f520e9b11. This is an aggressive leak which will cause a crash quickly once you have any significant throughput. – brainkim Sep 01 '20 at 20:43
  • 1
    Attempting to salvage answer. Feel free to add a competing answer with more info and I'll upvote. – jib Sep 01 '20 at 23:19
  • @brainkim Oh wow, if a memory snapshot can confirm that the strings are retained by `pending`, that's case 2 and is a serious bug indeed. I'd suggest you create a new issue on the V8 bug tracker for that. – Bergi Sep 01 '20 at 23:38
  • @Bergi I confirmed it with a V8 snapshot, and I’ll attempt to create a clean dump for the V8 bug tracker soon. My “I don’t know C++” understanding of the object graph is as follows. The pending promise retains the promise reaction created by Promise.race. The promise reaction retains the resolve and reject functions of the promise returned from race. The resolve and reject functions retain the promise which is returned from race. Finally, this promise retains the resolved value of the race, which is the big string. Makes sense if you think about it, but completely broken. – brainkim Sep 02 '20 at 00:03
  • @brainkim Yes, that would be my understanding. And the reference to the promise that the `resolve` and `reject` functions close over should be nulled out when the promise is resolved. – Bergi Sep 02 '20 at 00:08
  • @Bergi I wrote a writeup in the node.js issue of my current conclusions (https://github.com/nodejs/node/issues/17469#issuecomment-685216777), and included a safe version of Promise.race using some of your suggestions and a WeakMap. I don’t want to maintain a “safe-race” implementation on NPM; my ultimate goal is to get this fixed, especially because it has huge ramifications for async iterator use cases. – brainkim Sep 02 '20 at 01:19