Node: `Promise.race` lead to memory leak

[win-t]

• Version: v9.2.0 and v8.9.1, probably other version also affected
• Platform: linux
• Subsystem: -

I run this code inside docker with -m 100m --memory-swap 100m , and then the program crash after few second

function doWork() {
    // ... do real work with some sleep at the end, but for demo purpose i just return (async) resolved promise
    return new Promise(a => setImmediate(a));
}

var exit = false;
const exitPromise = new Promise(a => {
    process.once('SIGINT', () => {
        exit = true;
        a();
    });
});

(async () => {
    console.log('start');
    while(!exit) {
        await Promise.race([exitPromise, doWork()]);
    }
    console.log('stop');
})().catch(console.error.bind(console));

but it perfectly fine when i change await Promise.race([exitPromise, doWork()]); with await doWork();

[win-t]

problem with more shorter code:

var x = new Promise(() => {});
(async () => {
    while(true) {
        await Promise.race([x, Promise.resolve()]);
    }
})().catch(console.error.bind(console));

[vsemozhetbyt]

May it be that you just overflood the memory with many unresolved pending exitPromise Promises?

[win-t]

but, why?, I create exactly one exitPromise, and the purpose is to catch signal and tell the program to exit cleanly

[vsemozhetbyt]

Yes, sorry. FWIW, I cannot reproduce the crash on Windows 7 x64 with any of both scripts and Node.js v9.2.0. Can anybody reproduce on Linux?

[win-t]

@vsemozhetbyt maybe you need to wait for more time, maybe memory leak still occurs but you already stop the script before it crash,
I don't how to limit memory consumption in Windows, but in Linux we can use cgroup (that is what docker use when I tell -m 100m --memory-swap 100m)

[vsemozhetbyt]

The memory does grow fast, but then it GCed. I've waited some minutes without a crash.

[apapirovski]

Keep in mind you have to use --max-old-space-size to limit memory consumption, otherwise V8 has no way of knowing how much memory is available for safe use. Also the usual caveats to --max-old-space-size usage apply — it doesn't account for the full memory consumption (heap only, I believe) so setting it to 100 will not work.

Chances are it's just waiting to GC when it's less busy because it assumes a lot more memory is available than in reality.

[bnoordhuis]

I can confirm that there is a memory leak and I initially labeled it 'confirmed-bug' but turns it's spec-conforming and needs some explanation. First off, this code:

var x = new Promise(() => {});
for (;;) await Promise.race([x, Promise.resolve()]);

Is conceptually equivalent to, and produces the same memory usage pattern as:

var x = new Promise(() => {});
for (;;) await new Promise((resolve, reject) => {
  x.then(resolve, reject);
  Promise.resolve().then(resolve, reject);
});

That's more or less how V8's CodeStubAssembler implements Promise.race().

When you monkey-patch the built-in .then method, memory usage is stable:

var x = new Promise(() => {});
x.then = () => {};
for (;;) await Promise.race([x, Promise.resolve()]);

That's because the built-in .then appends the callbacks.

var x = new Promise(() => {});
%DebugPrint(x);
for (var i = 1024; --i > 0;) await Promise.race([x, Promise.resolve()]);
%DebugPrint(x);

Prints with a debug build (and --allow_natives_syntax):

...
 - deferred_promise: 0x18042c0822e1 <undefined>
 - deferred_on_resolve: 0x18042c0822e1 <undefined>
 - deferred_on_reject: 0x18042c0822e1 <undefined>
 - fulfill_reactions = 0x18042c0822e1 <undefined>
 - reject_reactions = 0x18042c0822e1 <undefined>
...
 - status = pending
...
 - deferred_promise: 0x180448e7d2c9 <FixedArray[1023]>
 - deferred_on_resolve: 0x18048c382201 <FixedArray[1023]>
 - deferred_on_reject: 0x18048c384209 <FixedArray[1023]>
 - fulfill_reactions = 0x18048c386211 <FixedArray[1023]>
 - reject_reactions = 0x18048c388219 <FixedArray[1023]>
...

The responsible code is here:
https://github.com/nodejs/node/blob/cd174df353e78cde9181299adbf501a4a694dee8/deps/v8/src/builtins/builtins-promise-gen.cc#L560-L573

From section 25.4.5.3.1 of https://tc39.github.io/ecma262/#sec-control-abstraction-objects:

If promise.[[PromiseState]] is "pending", then

Append fulfillReaction as the last element of the List that is promise.[[PromiseFulfillReactions]].
Append rejectReaction as the last element of the List that is promise.[[PromiseRejectReactions]].

For better or worse, it's the expected behavior. V8 implements the spec to the letter.

[fanatid]

I had studied how Promise implemented and works in V8. I found that Promise.race use Promise.prototype.then of promises which was passed to Promise.race, as result if promise never resolved and have reference memory will grow and GC can not collect it.
Then I googled and found this issue.
I understand that by spec as @bnoordhuis pointed reactions should be added to lists (V8 by the way have one list which contain both Fulfill and Reject), but probably these reactions should be weak and it's should be possible collect them if they do not have any other references or if they not affect anyhow on promise to which lists they was added?

Another example which show this:

let resolveUnresolved
const unresolved = new Promise((r) => { resolveUnresolved = r })
const resolved = Promise.resolve(42)

setInterval(() => {
  for (let i = 0; i < 1e5; ++i) {
    Promise.race([unresolved, resolved])
  }

  const { heapUsed } = process.memoryUsage()
  if (heapUsed > 500 * 1024 * 1024) resolveUnresolved()
}, 100)

Better run it with --trace-gc, so GC work can be observable. If resolveUnresolved call will be removed, script will fail. If called, then >500MiB Heap usage will be dropped to normal value.

[bnoordhuis]

probably these reactions should be weak and it's should be possible collect them if they do not have any other references or if they not affect anyhow on promise to which lists they was added?

I don't think that could work because then the reactions could disappear without running to completion (fulfill/reject) when the main promise is the only one holding a reference to them.

It _might_ be possible to weaken their references once they've ran to completion but that's something you'd have to take up with the V8 project. It probably needs something more advanced than the current list of arrays.

[fanatid]

Yeah, arrays definitely not for this, maybe linked list. Should I create issue in V8 project?

[bnoordhuis]

Yes, that would be best. Thanks.

[dko-slapdash]

@fanatid did you happen to create the V8 issue? Is there a link?

[fanatid]

https://bugs.chromium.org/p/v8/issues/detail?id=9858

[jeffijoe]

This still seems to be an issue, I am on 13.8, but also tested this on 14.1, same issue.

The following script will print memory deltas and is configurable for creating certain situations. The use case is long-lived cancellation tokens (for graceful shutdown).

The script will throw if any of the memory stats exceed 20% between start and end sample.

Example output when leaking:

Heap used: 24573872.00 delta (74.84%)
Heap total: 32772096.00 delta (65.44%)
RSS: 27291648.00 delta (39.34%)

Example outbput when not leaking:

Heap used: 9432.00 delta (0.40%)
Heap total: -262144.00 delta (-3.34%)
RSS: 2576384.00 delta (7.76%)

/*
  To run the sample that leaks (Promise.race()):

    node --expose-gc leak.js --leak

  To run the sample that has the same observable behavior but does not leak:

    node --expose-gc leak.js

  To customize at what iteration to take the start and end sample:

    node --expose-gc leak.js --startSampleAt=10000 --endSampleAt=50000

  To simulate a long-running iteration, use the following to tell the script when to make it a long work period.
  This is for testing the cancellation.

    node --expose-gc leak.js --workHardAt=420

*/
const assert = require("assert");
let sampleA, sampleB;

const { startSampleAt, endSampleAt, workHardAt, shouldLeak } = parseArgs();

main();

async function main() {
  console.log("Welcome to the Promise.race() memory leak sample!");
  console.log(
    shouldLeak
      ? "- Using Promise.race(); thicc heap incoming"
      : "- Using manual safe race"
  );
  console.log("- Will take the start sample at ", startSampleAt, "iterations");
  console.log("- Will take the end sample at ", endSampleAt, "iterations");
  if (workHardAt >= 0) {
    console.log(
      "- Will simulate working hard at ",
      workHardAt,
      "iterations. Use Ctrl+C to cancel it early and watch the console."
    );
  }

  const token = processTerminationToken();
  let i = 0;
  const progress = Progress(endSampleAt);
  while (!token.isTerminated) {
    progress.next(i);
    if (i === startSampleAt) {
      console.log("Taking start sample");
      maybeStartSample();
    }
    await race(doSomeAsyncWork(i === workHardAt, token), token);
    if (token.isTerminated) {
      console.log("Terminated after working at " + i);
    } else {
      await race(sleep(1), token);
    }
    i++;
    if (i > endSampleAt) {
      console.log("Taking end sample");
      maybePrintGcInfo();
      break;
    }
  }
  console.log("Exiting gracefully");
}

/**
 * Shorthand for racing with the requested approach.
 */
async function race(promise, token) {
  if (shouldLeak) {
    return Promise.race([promise, token.cancellation]);
  }
  return token.race(promise);
}

/**
 * Parses arguments for the script. Applies defaults.
 */
function parseArgs() {
  const START_SAMPLE_AT_FLAG = "--startSampleAt=";
  const END_SAMPLE_AT_FLAG = "--endSampleAt=";
  const WORK_HARD_AT_FLAG = "--workHardAt=";
  const shouldLeak = process.argv.includes("--leak");
  const startSampleAtArg = process.argv.find((a) =>
    a.startsWith(START_SAMPLE_AT_FLAG)
  );
  const endSampleAtArg = process.argv.find((a) =>
    a.startsWith(END_SAMPLE_AT_FLAG)
  );
  const workHardAtArg = process.argv.find((a) =>
    a.startsWith(WORK_HARD_AT_FLAG)
  );
  const startSampleAt = startSampleAtArg
    ? parseInt(startSampleAtArg.substring(START_SAMPLE_AT_FLAG.length), 10)
    : 1000;
  return {
    shouldLeak,
    startSampleAt,
    endSampleAt: endSampleAtArg
      ? parseInt(endSampleAt.substring(END_SAMPLE_AT_FLAG.length), 10)
      : startSampleAt * 5,
    workHardAt: workHardAtArg
      ? parseInt(workHardAtArg.substring(WORK_HARD_AT_FLAG.length), 10)
      : -1,
  };
}

async function doSomeAsyncWork(shouldWorkHard, token) {
  if (shouldWorkHard) {
    console.log("Working real hard");
    await race(sleep(5000, token), token);

    if (token.isTerminated) {
      console.log("Was working super hard but was cut short!");
    } else {
      console.log("Done working real hard");
    }
  }
  await sleep(1);
}

/**
 * Sleep func that cancels the timer when the token is cancelled.
 */
async function sleep(ms, token) {
  await new Promise((resolve) => {
    const dispose = token
      ? token.onTerminate(() => clearTimeout(timeout))
      : () => {};
    const timeout = setTimeout(() => {
      dispose();
      resolve();
    }, ms);
  });
}

/**
 * Creates a cancellation token based on the process receiving a `SIGINT`.
 * It's a basic cancellation token implementation.
 */
function processTerminationToken() {
  let isTerminated = false;
  const cancellation = new Promise((resolve) => {
    process.on("SIGINT", () => {
      isTerminated = true;
      console.log("Termination requested!");
      callbacks.forEach((c) => c());
      callbacks = null;
      resolve();
    });
  });
  let callbacks = [];
  const onTerminate = (handler) => {
    callbacks.push(handler);
    const dispose = () => {
      const idx = callbacks.indexOf(handler);
      if (idx > -1) {
        callbacks.splice(idx, 1);
      }
    };
    return dispose;
  };
  return {
    cancellation,
    onTerminate,
    get isTerminated() {
      return isTerminated;
    },
    race(promise) {
      return new Promise((resolve, reject) => {
        const dispose = onTerminate(resolve);
        promise.finally(dispose).then(resolve, reject);
      });
    },
  };
}

/**
 * Starts a sample if GC is exposed.
 */
function maybeStartSample() {
  if (typeof global.gc === "function") {
    global.gc();
    sampleA = process.memoryUsage();
  }
}

/**
 * Takes the end sample and prints it. Asserts that it didn't grow more than we expected it to.
 */
function maybePrintGcInfo() {
  if (typeof global.gc === "function") {
    global.gc();
    sampleB = process.memoryUsage();

    console.log(
      "Heap used: " + statToString(sampleA.heapUsed, sampleB.heapUsed)
    );
    console.log(
      "Heap total: " + statToString(sampleA.heapTotal, sampleB.heapTotal)
    );
    console.log("RSS: " + statToString(sampleA.rss, sampleB.rss));

    console.log("Memory usage at start:");
    console.dir(sampleA);
    console.log("Memory usage at end:");
    console.dir(sampleB);

    assert(
      sampleA.rss * 1.2 > sampleB.rss,
      "RSS should not grow by more than 20%"
    );
    assert(
      sampleA.heapTotal * 1.2 > sampleB.heapTotal,
      "heapTotal should not grow by more than 20%"
    );
    assert(
      sampleA.heapUsed * 1.2 > sampleB.heapUsed,
      "heapUsed should not grow by more than 20%"
    );
  } else {
    console.log("need to run node with --expose-gc to view GC stats");
  }
}

function statToString(start, finish) {
  const delta = Math.round(finish - start);
  const percent = (delta / finish) * 100;
  return `${delta.toFixed(2)} delta (${percent.toFixed(2)}%)`;
}

function Progress(max) {
  let prev = 0;
  let done = false;
  return {
    next(i) {
      if (done) {
        return;
      }
      const percent = Math.round((i / max) * 100);
      if (percent > prev) {
        prev = percent;
        console.log(`${percent}% (${i} / ${max})`);
      }
      if (percent === 100) {
        done = true;
        process.stdout.write("\n");
        console.log("Done!");
      }
      prev = percent;
    },
  };
}

[brainkim]

Hi, given that this is currently the second search result on Google for Promise.race memory leak, I’m going to dump the following info here for anyone looking to fix a Promise.race-related memory leak. I apologize in advance for using the Node.js issue tracker like so.

Diagnosis

The leak described here isn’t just that an increasing number of promise reactions are created for a non-settling promise; this would be more gradual and difficult to detect. In actuality, when you call Promise.race with a long-running promise, the resolved value of the returned promise gets retained for as long as each of its promises do not settle. This means that the severity of this leak is determined by whatever else you’re passing to Promise.race, and a high throughput will crash the process quickly.

Example code to demonstrate:

async function randomString(length) {
  await new Promise((resolve) => setTimeout(resolve, 1));
  let result = "";
  const characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
  for (let i = 0; i < length; i++) {
    result += characters.charAt(Math.floor(Math.random() * characters.length));
  }
  return result;
}

(async function main() {
  let i = 0;
  const pending = new Promise(() => {});
  while (true) {
    // We use random strings to prevent string interning.
    // Pass a different length string to see effects on memory usage.
    await Promise.race([pending, randomString(10000)]);
    if (i++ % 1000 === 0) {
      const usage = process.memoryUsage();
      const rss = Math.round(usage.rss / (1024 ** 2) * 100) / 100;
      const heapUsed = Math.round(usage.heapUsed / (1024 ** 2) * 100) / 100;
      console.log(`RSS: ${rss} MiB, Heap Used: ${heapUsed} MiB`);
    }
  }
})();

In this example, we pass a large random string along with a non-settling promise to Promise.race, and the result is that every single one of these strings is retained. The severity of this leak varies directly with the length passed to randomString. You can confirm that the problem is Promise.race and not the randomString function by replacing the Promise.race call with a direct call to randomString.

Calling Promise.race with the same promise is inherently unsafe because under the hood, we create a new promise reaction for each call, regardless of whether the promise has been seen before. This reaction retains the resolved value of the resulting promise. Promise.race is more or less equivalent to the following shim:

Promise.race = function(values) {
  return new Promise((resolve, reject) => {
    for (const value of values) {
      Promise.resolve(value).then(resolve, reject);
    }
  });
}

My understanding is that Promise.race causes unsettled promises to retain the resolved value of the returned promise via the following reference chain:

1. The unsettled promise retains
2. the promise reaction (internal object created when calling then on the unsettled promise), which retains
3. the resolve and reject functions of the promise returned from Promise.race, which retain
4. the promise returned from Promise.race, which retains
5. the resolved value of Promise.race.

You can confirm that the leak follows this path by adding a Promise.resolve() to the array of values passed to race in the original example:

(async function main() {
  let i = 0;
  const pending = new Promise(() => {});
  while (true) {
    const randomStringP = randomString(10000);
    // adding Promise.resolve() here so `Promise.race` fulfills to undefined.
    await Promise.race([Promise.resolve(), pending, randomStringP]);
    // We await the randomString promise because otherwise we would be creating 10000 length random strings
    // at the speed of the microtask queue, which would itself leak memory.
    await randomStringP;
    if (i++ % 1000 === 0) {
      const usage = process.memoryUsage();
      const rss = Math.round(usage.rss / (1024 ** 2) * 100) / 100;
      const heapUsed = Math.round(usage.heapUsed / (1024 ** 2) * 100) / 100;
      console.log(`RSS: ${rss} MiB, Heap Used: ${heapUsed} MiB`);
    }
  }
})();

If you run this example, you’ll see that there is still a leak, but it’s much less severe, indicating that what’s being retained each iteration is the Promise.resolve(), not randomStrings().

Fixing the leak

To fix the leak, we have to call then only once per individual promise passed to Promise.race. When you’re refactoring code, this pretty much means avoiding Promise.race entirely and replacing your await Promise.race expressions with await new Promise expressions, where the newly constructed Promise sets a function variable in the outer scope. This is difficult to explain with words so here is the leak reproduction so refactored:

(async function main() {
  let i = 0;
  // these functions are defined in a promise constructor later
  let resolve;
  let reject;
  const pending = new Promise(() => {});
  // This call to `then` is not necessary here, but shows how you would listen to a long-running promise.
  // Note that we call the `then` method with anonymous functions which close over resolve and reject;
  // simply passing resolve and reject would not work.
  pending.then((value) => resolve(value), (err) => reject(err));
  while (true) {
    // We again call the then method directly with anonymous functions which close over resolve and reject.
    randomStrings(10000).then((value) =>  resolve(value), (err) => reject(err));
    // This is the await call which replaces the `await Promise.race` expression in the leaking example.
    await new Promise((resolve1, reject1) => {
      resolve = resolve1;
      reject = reject1;
    });
    if (i++ % 1000 === 0) {
      const usage = process.memoryUsage();
      const rss = Math.round(usage.rss / (1024 ** 2) * 100) / 100;
      const heapUsed = Math.round(usage.heapUsed / (1024 ** 2) * 100) / 100;
      console.log(`RSS: ${rss} MiB, Heap Used: ${heapUsed} MiB`);
    }
  }
})();

If you run the code as modified, you’ll see that memory usage levels off after a while, even though it has the same behavior. The connection between the unsettled promise and each randomString iteration has been broken. The key detail here is the use of anonymous functions which close over the resolve and reject functions of the returned promise; this is what allows the unsettled promise to only weakly reference the awaited promise.

Can we abstract this logic into a replacement for Promise.race? Yes, and we can use a WeakMap to do so. A high-level overview of the algorithm is that we keep track of every value passed to race, and call then on each value at most once. To handle multiple calls with the same promise, we use the value as a key to a WeakMap, which stores a set of deferreds for each individual race call. If a value settles, we find all of its related deferreds and invoke them appropriately. If a race settles, we find all sets stored in the WeakMap for each of the raced values, and delete the race’s deferred from each set. Here is the full implementation of this algorithm:

function isPrimitive(value) {
  return (
    value === null ||
    (typeof value !== "object" && typeof value !== "function")
  );
}

// Keys are the values passed to race, values are a record of data containing a
// set of deferreds and whether the value has settled.
const wm = new WeakMap();
function safeRace(contenders) {
  let deferred;
  const result = new Promise((resolve, reject) => {
    deferred = {resolve, reject};
    for (const contender of contenders) {
      if (isPrimitive(contender)) {
        // If the contender is a primitive, attempting to use it as a key in the
        // weakmap would throw an error. Luckily, it is safe to call
        // `Promise.resolve(contender).then` on a primitive value multiple times
        // because the promise fulfills immediately.
        Promise.resolve(contender).then(resolve, reject);
        continue;
      }

      let record = wm.get(contender);
      if (record === undefined) {
        record = {deferreds: new Set([deferred]), settled: false};
        wm.set(contender, record);
        // This call to `then` happens once for the lifetime of the value.
        Promise.resolve(contender).then(
          (value) => {
            for (const {resolve} of record.deferreds) {
              resolve(value);
            }

            record.deferreds.clear();
            record.settled = true;
          },
          (err) => {
            for (const {reject} of record.deferreds) {
              reject(err);
            }

            record.deferreds.clear();
            record.settled = true;
          },
        );
      } else if (record.settled) {
        // If the value has settled, it is safe to call
        // `Promise.resolve(contender).then` on it.
        Promise.resolve(contender).then(resolve, reject);
      } else {
        record.deferreds.add(deferred);
      }
    }
  });

  // The finally callback executes when any value settles, preventing any of
  // the unresolved values from retaining a reference to the resolved value.
  return result.finally(() => {
    for (const contender of contenders) {
      if (!isPrimitive(contender)) {
        const record = wm.get(contender);
        record.deferreds.delete(deferred);
      }
    }
  });
}

If you replace Promise.race with the safeRace function in the initial leak reproduction, you’ll see that it stops leaking. On my Macbook Air, the resident set size expands to ~70MiB before V8 figures out what’s going on.

Conclusion

I will file another issue or leave a comment on the V8 issue tracker, because this is unacceptable behavior for Promise.race. The memory behavior of safeRace above is pretty much what most developers expect from the runtime, and I don’t know why it doesn’t behave this way. I don’t think saying that the current implementation is to spec is a good enough answer. I release all of the code above as public domain with the hope that you include a link to this comment so that this issue gets more attention. Alternatively, feel free to ping me or email me about specific Promise.race refactorings and I will be glad to help.

[bergus]

@brainkim That WeakMap solution is totally overcomplicated (tbh I didn't even spend the time to fully comprehend it). All you need to break the reference chain is

function safeResolverPromise(executor) {
  return new Promise((resolve, reject) => {
    executor(res => {
      if (resolve) resolve(res);
      resolve = reject = null;
    }, err => {
      if (reject) reject(err);
      resolve = reject = null;
    });
  });
});

function safeRace(values) {
  return safeResolverPromise((resolve, reject) => {
    for (const value of values) {
      Promise.resolve(value).then(resolve, reject);
    }
  });
}

That said, I would expect this garbage collection behaviour from the resolver functions of any promise implementation, but especially a native one. I agree this is unacceptable and hope it will get fixed in V8 soon.

[brainkim]

@bergus

That WeakMap solution is totally overcomplicated

Have you tested your example against my leak reproduction? It blows the heap immediately just like Promise.race:

async function randomString(length: number) {
  await new Promise((resolve) => setTimeout(resolve, 1));
  let result = "";
  const characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
  for (let i = 0; i < length; i++) {
    result += characters.charAt(Math.floor(Math.random() * characters.length));
  }
  return result;
}

function safeResolverPromise(executor) {
  return new Promise((resolve, reject) => {
    executor(res => {
      if (resolve) resolve(res);
      resolve = reject = null;
    }, err => {
      if (reject) reject(err);
      resolve = reject = null;
    });
  });
}

function safeRace(values) {
  return safeResolverPromise((resolve, reject) => {
    for (const value of values) {
      Promise.resolve(value).then(resolve, reject);
    }
  });
}

(async function main() {
  let i = 0;
  const pending = new Promise(() => {});
  while (true) {
    await safeRace([pending, randomString(10000)]);
    if (i++ % 1000 === 0) {
      const usage = process.memoryUsage();
      const rss = Math.round(usage.rss / (1024 ** 2) * 100) / 100;
      const heapUsed = Math.round(usage.heapUsed / (1024 ** 2) * 100) / 100;
      console.log(`RSS: ${rss} MiB, Heap Used: ${heapUsed} MiB`);
    }
  }
})();

You can knock my WeakMap solution for being too complicated, but I can tell you from experience staring into the bowels of the microtask queue that it’s about as complicated as necessary.