Promise.race lead to memory leak
#17469
Comments
|
problem with more shorter code: var x = new Promise(() => {});
(async () => {
while(true) {
await Promise.race([x, Promise.resolve()]);
}
})().catch(console.error.bind(console)); |
|
May it be that you just overflood the memory with many unresolved pending |
|
but, why?, I create exactly one |
|
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? |
|
@vsemozhetbyt maybe you need to wait for more time, maybe memory leak still occurs but you already stop the script before it crash, |
|
The memory does grow fast, but then it GCed. I've waited some minutes without a crash. |
mscdex
added
memory
|
Keep in mind you have to use 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
added
confirmed-bug
|
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 When you monkey-patch the built-in var x = new Promise(() => {});
x.then = () => {};
for (;;) await Promise.race([x, Promise.resolve()]);That's because the built-in 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 The responsible code is here: node/deps/v8/src/builtins/builtins-promise-gen.cc Lines 560 to 573 in cd174df From section 25.4.5.3.1 of https://tc39.github.io/ecma262/#sec-control-abstraction-objects:
For better or worse, it's the expected behavior. V8 implements the spec to the letter. |
`Promise.race()` leads to memory leaks if some promises are never resolved. See nodejs/node#17469
|
I had studied how 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 |
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. |
|
Yeah, arrays definitely not for this, maybe linked list. Should I create issue in V8 project? |
|
Yes, that would be best. Thanks. |
|
@fanatid did you happen to create the V8 issue? Is there a link? |
|
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: Example outbput when not leaking: /*
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;
},
};
} |
|
(Updated on Jun 3rd, 2024 to include fix for leaking closures by @danfuzz.) Hi, given that this is currently the second search result on Google for DiagnosisThe 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 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 Calling Promise.race = function(values) {
return new Promise((resolve, reject) => {
for (const value of values) {
Promise.resolve(value).then(resolve, reject);
}
});
}My understanding is that
You can confirm that the leak follows this path by adding a (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 Fixing the leakTo fix the leak, we have to call (async function main() {
let i = 0;
// these functions are set 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 because they are currently undefined.
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.
randomString(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 Can we abstract this logic into a replacement for /*
This is free and unencumbered software released into the public domain.
Anyone is free to copy, modify, publish, use, compile, sell, or
distribute this software, either in source code form or as a compiled
binary, for any purpose, commercial or non-commercial, and by any
means.
In jurisdictions that recognize copyright laws, the author or authors
of this software dedicate any and all copyright interest in the
software to the public domain. We make this dedication for the benefit
of the public at large and to the detriment of our heirs and
successors. We intend this dedication to be an overt act of
relinquishment in perpetuity of all present and future rights to this
software under copyright law.
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR
OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
OTHER DEALINGS IN THE SOFTWARE.
For more information, please refer to <http://unlicense.org/>
*/
function isPrimitive(value) {
return (
value === null ||
(typeof value !== "object" && typeof value !== "function")
);
}
function addRaceContender(contender) {
const deferreds = new Set();
const record = {deferreds, settled: false};
// This call to `then` happens once for the lifetime of the value.
Promise.resolve(contender).then(
(value) => {
for (const {resolve} of deferreds) {
resolve(value);
}
deferreds.clear();
record.settled = true;
},
(err) => {
for (const {reject} of deferreds) {
reject(err);
}
deferreds.clear();
record.settled = true;
},
);
return record;
}
// Keys are the values passed to race, values are a record of data containing a
// set of deferreds and whether the value has settled.
/** @type {WeakMap<object, {deferreds: Set<Deferred>, settled: boolean}>} */
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 = addRaceContender(contender);
record.deferreds.add(deferred);
wm.set(contender, record);
} 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 ConclusionI will file another issue or leave a comment on the V8 issue tracker, because this is unacceptable behavior for |
|
@brainkim That 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. |
Have you tested your example against my leak reproduction? It blows the heap immediately just like 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. |
This PR imports and adapts the "safe `Promise.race()`" method, as written by Brian Kim (@brainkim), for use in this project. This is a workaround for a bug in V8 which causes perfectly cromulent uses of `Promise.race()` to leak memory (specifically, cases where a long-running system runs repeated races involving a long-unresolved promise). Links are included near the implementation, but FWIW see these for more detail: * <nodejs/node#17469 (comment)> * <https://bugs.chromium.org/p/v8/issues/detail?id=9858> In this codebase, the upshot of the bug is that (almost?) every event that got logged was leaked, along with a bit of related "connective tissue." Many many thanks to Brian for writing the code in question, and explaining the issue thoroughly.
|
Just FYI, I ran into this issue in my project recently and used @brainkim's TLDR: The call to |
|
Having faced memory leaks in a project with long-lived Promises, I explored a different pattern to the one in Update: The solution is now also published on npm at @watchable/unpromise Instead of having a race-specific mechanism, I created a minimal wrapper around The resulting implementation of Unpromise.race() ends up quite intuitive, and I believe prevents memory leaks too... /** Perform Promise.race via SubscribedPromises, then unsubscribe them.
* Equivalent to Promise.race but eliminates memory leaks from long-lived
* promises accumulating .then() and .catch() subscribers. */
static async race<T extends readonly unknown[] | []>(
values: T
): Promise<Awaited<T[number]>>;
static async race<T>(
values: Iterable<T | PromiseLike<T>>
): Promise<Awaited<T>> {
const valuesArray = Array.isArray(values) ? values : [...values];
const subscribedPromises = valuesArray.map(Unpromise.resolve);
try {
return await Promise.race(subscribedPromises);
} finally {
subscribedPromises.forEach(({ unsubscribe }) => {
unsubscribe();
});
}
}A by-product of the approach is that a memory-safe Unpromise.any() is similarly intuitive... /** Perform Promise.any() via SubscribedPromises, then unsubscribe them.
* Equivalent to Promise.any but eliminates memory leaks from long-lived
* promises accumulating .then() and .catch() subscribers. */
static async any<T extends readonly unknown[] | []>(
values: T
): Promise<Awaited<T[number]>>;
static async any<T>(
values: Iterable<T | PromiseLike<T>>
): Promise<Awaited<T>> {
const valuesArray = Array.isArray(values) ? values : [...values];
const subscribedPromises = valuesArray.map(Unpromise.resolve);
try {
return await Promise.any(subscribedPromises);
} finally {
subscribedPromises.forEach(({ unsubscribe }) => {
unsubscribe();
});
}
}I believe The complete implementation of The basic approach is to hand off to the platform Promise implementation for all the logic, but to 'shadow' any Promise with a single, equivalently-long-lived proxy ( Unpromise ), cached in a WeakMap for the lifetime of the original Promise. Every promise call to this Unpromise returns a Promise with an Promises arising from Unpromise transforms seem to pass a basic Promise compliance suite.
Have I failed to consider something important that might still expose me to memory leaks or other problems? |
|
Since this problem isn't going away, I published Unpromise as an npm package... Docs at https://watchable.dev/api/modules/_watchable_unpromise.html I welcome issue feedback on the package via e.g. https://github.com/cefn/watchable/issues or discussion via email via https://cefn.com |
|
I may be missing something but I don't believe that the WeakMap used in @brainkim's solution is necessary — the following works for me and alleviates the repro's memory leak in my testing (checked on Node v18.17.0): /** By Sophie Alpert (2024), released into the public domain or under the terms of https://unlicense.org/ */
function resolveWithResolvers(resolvers, value) {
if (resolvers.resolve) {
resolvers.resolve(value);
resolvers.resolve = resolvers.reject = null;
}
}
function rejectWithResolvers(resolvers, value) {
if (resolvers.reject) {
resolvers.reject(value);
resolvers.resolve = resolvers.reject = null;
}
}
Promise.race = function race(values) {
return new Promise((resolve, reject) => {
const resolvers = { resolve, reject };
const resolve2 = resolveWithResolvers.bind(null, resolvers);
const reject2 = rejectWithResolvers.bind(null, resolvers);
for (const value of values) {
Promise.resolve(value).then(resolve2, reject2);
}
});
}; |
While nulling the resolvers prevents the "large" size of the leak, aka any pending raced promises holding onto the resolution value of a race it was a part of, there is still a problem that any reaction added to raced promises will accumulate until the promise is settled, even though these reactions became unnecessary. In this case your |
|
Sure, acknowledged. If you are passing the same promise many times to Promise.race then the WeakMap solution is slightly more memory-efficient. If the promises are being created anew then it is constant overhead. The hash lookups in the WeakMap also incur a runtime tax, so neither option is without downsides. |
FWIW this is exactly the pattern in my code which triggered the problem with the built-in [Updated to add] For me at least, having the contract of the regular |
|
Co-racing termination promises was exactly the scenario that prompted me to create https://watchable.dev/api/modules/_watchable_unpromise.html and it is the worked example in the Medium article introducing it... https://levelup.gitconnected.com/unfixable-memory-leaks-in-promise-race-28e5c5a6032c For this scenario there seems to be an unstoppable memory leak and who-knows-what performance issues as the billionth |
# Summary If a member of `Promise.race` never settles, every promise in the race will be retained indefinitely. This causes memory leaks. See nodejs/node#17469 for much more information. In this PR we fork the excellent work of @brainkim, @danfuzz, and @szakharchenko; a ‘safe’ version of `race` that ensures that each member of the race settles when the winner settles. # Test Plan See next PR in this stack Addresses #3069.
I run this code inside docker with
-m 100m --memory-swap 100m, and then the program crash after few secondbut it perfectly fine when i change
await Promise.race([exitPromise, doWork()]);withawait doWork();The text was updated successfully, but these errors were encountered: