Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
26 changes: 14 additions & 12 deletions src/jsc/bindings/WriteBarrierList.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,20 @@
}
}

// Move every element into `arguments` and clear the backing vector in one
// pass under a single cellLock. Prefer this over a takeFirst() loop, which
// is O(n^2) (Vector::removeAt(0) memmove + per-element lock acquire).
void drainTo(JSC::JSCell* owner, JSC::MarkedArgumentBuffer& arguments)
{
WTF::Locker locker { owner->cellLock() };
arguments.ensureCapacity(arguments.size() + m_list.size());
for (JSC::WriteBarrier<T>& value : m_list) {
if (auto* cell = value.get())
arguments.append(cell);
}
m_list.clear();
}

Check warning on line 67 in src/jsc/bindings/WriteBarrierList.h

View check run for this annotation

Claude / Claude Code Review

drainTo() near-duplicates existing moveTo()

The new `drainTo()` is nearly identical to `moveTo()` just above it — same signature, same `cellLock()`, same loop appending non-null cells to a `MarkedArgumentBuffer` — except `drainTo()` is strictly better (adds `ensureCapacity()` and actually empties `m_list` instead of leaving N nulled `WriteBarrier`s behind). `moveTo()`'s sole caller (`JSBundlerPlugin.cpp:670`) works fine with `drainTo()` semantics, so consider just switching that caller to `drainTo()` and deleting `moveTo()` (or upgrading
Comment thread
claude[bot] marked this conversation as resolved.

template<typename Visitor>
void visit(JSC::JSCell* owner, Visitor& visitor)
{
Expand All @@ -66,18 +80,6 @@
return m_list.isEmpty();
}

T* takeFirst(JSC::JSCell* owner)
{
WTF::Locker locker { owner->cellLock() };
if (m_list.isEmpty()) {
return nullptr;
}

T* value = m_list.first().get();
m_list.removeAt(0);
return value;
}

template<typename MatchFunction>
bool removeFirstMatching(JSC::JSCell* owner, const MatchFunction& matches)
{
Expand Down
35 changes: 26 additions & 9 deletions src/jsc/bindings/ZigGlobalObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3278,18 +3278,35 @@

void GlobalObject::handleRejectedPromises()
{
if (m_aboutToBeNotifiedRejectedPromises.isEmpty()) [[likely]]
return;

JSC::VM& virtual_machine = vm();
auto scope = DECLARE_TOP_EXCEPTION_SCOPE(virtual_machine);
while (auto* promise = m_aboutToBeNotifiedRejectedPromises.takeFirst(this)) {
if (promise->isHandled())
continue;

Bun__handleRejectedPromise(this, promise);
if (auto ex = scope.exception()) {
(void)scope.tryClearException();
this->reportUncaughtExceptionAtEventLoop(this, ex);
do {
// Move the whole list out under one cellLock, then iterate linearly.
// The previous takeFirst() loop did Vector::removeAt(0) + cellLock per
// element, which is O(n^2) for n queued rejections. JSC's
// VM::didExhaustMicrotaskQueue and WebCore's RejectedPromiseTracker
// both use the same move-out-then-iterate pattern.

Check warning on line 3291 in src/jsc/bindings/ZigGlobalObject.cpp

View check run for this annotation

Claude / Claude Code Review

Comments narrate deleted code / bug history

nit: the comments here narrate the deleted `takeFirst()` implementation ("The previous takeFirst() loop did Vector::removeAt(0)…O(n^2)", "matches the original takeFirst loop's semantics…"), which root `CLAUDE.md` and `src/CLAUDE.md` say belongs in the PR description, not in code. Trim to the durable parts — the JSC/WebCore precedent reference and "An unhandledRejection handler may itself reject a promise; loop until the list stays empty". Same for `WriteBarrierList.h:56-57` ("Prefer this over a
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
JSC::MarkedArgumentBuffer promises;
m_aboutToBeNotifiedRejectedPromises.drainTo(this, promises);
RELEASE_ASSERT(!promises.hasOverflowed());
for (size_t i = 0, size = promises.size(); i < size; ++i) {
auto* promise = static_cast<JSC::JSPromise*>(promises.at(i).asCell());
if (promise->isHandled())
continue;

Check failure on line 3298 in src/jsc/bindings/ZigGlobalObject.cpp

View check run for this annotation

Claude / Claude Code Review

Spurious rejectionHandled event when promise is .catch()'d from inside unhandledRejection handler

This introduces a behavioral regression: because `drainTo()` empties `m_aboutToBeNotifiedRejectedPromises` *before* any handler runs, if an `unhandledRejection` handler synchronously `.catch()`s another still-queued rejected promise, `promiseRejectionTracker(..., Handle)` now finds the list empty and falls through to `Bun__handleHandledPromise` — firing a spurious `'rejectionHandled'` event (and `PromiseRejectionHandledWarning`) for a promise that never emitted `'unhandledRejection'`. The old `t
Comment thread
claude[bot] marked this conversation as resolved.

Bun__handleRejectedPromise(this, promise);
if (auto ex = scope.exception()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What if its a termination exception? tryClearException will return false and what we should do, in that case, is return out of this function.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@robobun can you resume this PR

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done in 8aec644: checks vm.isTerminationException(ex) before tryClearException() and returns early, matching drainMicrotasks() in this file and VM::didExhaustMicrotaskQueue() upstream. The stack-scoped SetForScope restores m_rejectedPromisesBeingProcessed on the early return.

(void)scope.tryClearException();
this->reportUncaughtExceptionAtEventLoop(this, ex);
}
}
}
// An unhandledRejection handler may itself reject a promise; loop
// until the list stays empty (matches the original takeFirst loop's
// semantics, which re-read m_list each iteration).
} while (!m_aboutToBeNotifiedRejectedPromises.isEmpty());
}

DEFINE_VISIT_CHILDREN(GlobalObject);
Expand Down
38 changes: 38 additions & 0 deletions test/js/node/process/process.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,44 @@ describe.concurrent(() => {
expect(await proc.exited).toBe(42);
});

it("delivers many unhandledRejections in order, including ones queued from the handler", async () => {
// handleRejectedPromises drains the pending-rejection list in one O(n) pass
// (instead of takeFirst() per element). This test pins the observable
// behaviour: order is preserved, late .catch() suppresses delivery, and a
// rejection raised from inside the handler is also delivered.
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const N = 1000;
const seen = [];
let nestedSeen = false;
process.on("unhandledRejection", reason => {
if (reason === "nested") { nestedSeen = true; return; }
seen.push(reason);
if (reason === 0) Promise.reject("nested");
});
for (let i = 0; i < N; i++) Promise.reject(i);
// This one is handled before the checkpoint runs — must NOT be delivered.
Promise.reject("handled").catch(() => {});
await new Promise(r => setImmediate(r));
await new Promise(r => setImmediate(r));
if (seen.length !== N) throw new Error("count " + seen.length);
for (let i = 0; i < N; i++) if (seen[i] !== i) throw new Error("order at " + i + " got " + seen[i]);
if (seen.includes("handled")) throw new Error("handled promise was delivered");
if (!nestedSeen) throw new Error("rejection from inside handler was dropped");
console.log("ok");
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ stdout: "ok", stderr: "", exitCode: 0 });
});

it("aborts when the uncaughtException handler throws", async () => {
const proc = Bun.spawn([bunExe(), join(import.meta.dir, "process-onUncaughtExceptionAbort.js")], {
stderr: "pipe",
Expand Down
Loading