Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions lib/internal/process/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ const {
promiseRejectEvents: {
kPromiseRejectWithNoHandler,
kPromiseHandlerAddedAfterReject,
kPromiseRejectAfterResolved,
kPromiseResolveAfterResolved,
},
setPromiseRejectCallback,
} = internalBinding('task_queue');
Expand Down Expand Up @@ -161,21 +159,15 @@ function promiseRejectHandler(type, promise, reason) {
if (unhandledRejectionsMode === undefined) {
unhandledRejectionsMode = getUnhandledRejectionsMode();
}
// kPromiseRejectAfterResolved and kPromiseResolveAfterResolved are
// filtered out in C++ (src/node_task_queue.cc) and never reach JS.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a harm from keeping the cases and panicking if they’re hit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kPromiseRejectAfterResolved and kPromiseResolveAfterResolved are no longer even defined on the JS side after this patch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They also never worked

switch (type) {
case kPromiseRejectWithNoHandler: // 0
unhandledRejection(promise, reason);
break;
case kPromiseHandlerAddedAfterReject: // 1
handledRejection(promise);
break;
case kPromiseRejectAfterResolved: // 2
// Do nothing in this case. Previous we would emit a multipleResolves
// event but that was deprecated then later removed.
break;
case kPromiseResolveAfterResolved: // 3
// Do nothing in this case. Previous we would emit a multipleResolves
// event but that was deprecated then later removed.
break;
}
}

Expand Down
13 changes: 7 additions & 6 deletions src/node_task_queue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@ void PromiseRejectCallback(PromiseRejectMessage message) {

if (env == nullptr || !env->can_call_into_js()) return;

// multipleResolves was removed in v25 (PR #58707). Skip all work for
// these events to avoid OOM in tight Promise.race() loops (#51452).
if (event == kPromiseResolveAfterResolved ||
event == kPromiseRejectAfterResolved) {
return;
}

Local<Function> callback = env->promise_reject_callback();
// The promise is rejected before JS land calls SetPromiseRejectCallback
// to initializes the promise reject callback during bootstrap.
Expand All @@ -77,10 +84,6 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
"rejections",
"unhandled", unhandledRejections,
"handledAfter", rejectionsHandledAfter);
} else if (event == kPromiseResolveAfterResolved) {
value = message.GetValue();
} else if (event == kPromiseRejectAfterResolved) {
value = message.GetValue();
} else {
return;
}
Expand Down Expand Up @@ -173,8 +176,6 @@ static void Initialize(Local<Object> target,
Local<Object> events = Object::New(isolate);
NODE_DEFINE_CONSTANT(events, kPromiseRejectWithNoHandler);
NODE_DEFINE_CONSTANT(events, kPromiseHandlerAddedAfterReject);
NODE_DEFINE_CONSTANT(events, kPromiseResolveAfterResolved);
NODE_DEFINE_CONSTANT(events, kPromiseRejectAfterResolved);

target->Set(env->context(),
FIXED_ONE_BYTE_STRING(isolate, "promiseRejectEvents"),
Expand Down
24 changes: 24 additions & 0 deletions test/parallel/test-promise-race-memory-leak.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Flags: --max-old-space-size=64
'use strict';

// Regression test for https://github.com/nodejs/node/issues/51452
// When Promise.race() settles, V8 fires kPromiseResolveAfterResolved /
// kPromiseRejectAfterResolved for each "losing" promise. Before this fix,
// the C++ PromiseRejectCallback crossed into JS for these no-op events,
// accumulating references and causing OOM in tight async loops.
// With --max-old-space-size=64, this test would crash before completing
// if the leak is present.

const common = require('../common');

async function main() {
for (let i = 0; i < 100_000; i++) {
await Promise.race([
Promise.resolve(1),
Promise.resolve(2),
Promise.resolve(3),
]);
}
}

main().then(common.mustCall());