`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:

node/deps/v8/src/builtins/builtins-promise-gen.cc

Lines 560 to 573 in cd174df

	BIND(&if_multiplecallbacks);
	{
	AppendPromiseCallback(JSPromise::kDeferredPromiseOffset, promise,
	deferred_promise);
	AppendPromiseCallback(JSPromise::kDeferredOnResolveOffset, promise,
	deferred_on_resolve);
	AppendPromiseCallback(JSPromise::kDeferredOnRejectOffset, promise,
	deferred_on_reject);
	AppendPromiseCallback(JSPromise::kFulfillReactionsOffset, promise,
	var_on_resolve.value());
	AppendPromiseCallback(JSPromise::kRejectReactionsOffset, promise,
	var_on_reject.value());
	Goto(&out);
	}

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.