Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion src/jsc/bindings/JSBundlerPlugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,7 @@ extern "C" void JSBundlerPlugin__drainDeferred(Bun::JSBundlerPlugin* pluginObjec
{
auto* globalObject = pluginObject->globalObject();
MarkedArgumentBuffer arguments;
pluginObject->plugin.deferredPromises.moveTo(pluginObject, arguments);
pluginObject->plugin.deferredPromises.drainTo(pluginObject, arguments);
ASSERT(!arguments.hasOverflowed());

auto& vm = pluginObject->vm();
Expand Down
22 changes: 6 additions & 16 deletions src/jsc/bindings/WriteBarrierList.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,17 @@ class WriteBarrierList {
return m_list.mutableSpan();
}

void moveTo(JSC::JSCell* owner, JSC::MarkedArgumentBuffer& arguments)
// Move every element into `arguments` and clear the backing vector in one
// linear pass under a single cellLock.
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()) {
if (auto* cell = value.get())
arguments.append(cell);
value.clear();
}
}
m_list.clear();
}
Comment thread
claude[bot] marked this conversation as resolved.

template<typename Visitor>
Expand All @@ -66,18 +68,6 @@ class WriteBarrierList {
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
51 changes: 42 additions & 9 deletions src/jsc/bindings/ZigGlobalObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1053,6 +1053,18 @@ void GlobalObject::promiseRejectionTracker(JSGlobalObject* obj, JSC::JSPromise*
return unhandledPromise.get() == promise;
});
if (removed) break;
// handleRejectedPromises() drains the list into a local buffer before
// running any handler. A handler may .catch() a later still-queued
// promise; that promise is no longer in m_aboutToBeNotifiedRejectedPromises
// but has not yet had 'unhandledRejection' fired, so it must not get
// 'rejectionHandled'. Check every in-flight tail (handlers can re-enter
// handleRejectedPromises(), so there may be more than one).
for (auto* inflight = globalObj->m_rejectedPromisesBeingProcessed; inflight; inflight = inflight->outer) {
for (size_t i = inflight->index, n = inflight->buffer->size(); i < n; ++i) {
if (inflight->buffer->at(i).asCell() == promise)
return;
}
}
// The promise rejection has already been notified, now we need to queue it for the rejectionHandled event
Bun__handleHandledPromise(globalObj, promise);
break;
Expand Down Expand Up @@ -3278,18 +3290,39 @@ extern "C" void Bun__handleRejectedPromise(Zig::GlobalObject* JSGlobalObject, JS

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 same pattern JSC's VM::didExhaustMicrotaskQueue and WebCore's
// RejectedPromiseTracker use.
JSC::MarkedArgumentBuffer promises;
m_aboutToBeNotifiedRejectedPromises.drainTo(this, promises);
RELEASE_ASSERT(!promises.hasOverflowed());
// Expose the not-yet-processed tail so promiseRejectionTracker(Handle)
// can tell "still pending" apart from "already notified". Linked as a
// stack so a re-entrant handleRejectedPromises() (a handler that ticks
// the event loop) restores the outer frame instead of nulling it.
InFlightRejections inflight { &promises, 0, m_rejectedPromisesBeingProcessed };
WTF::SetForScope inflightScope(m_rejectedPromisesBeingProcessed, &inflight);
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;
Comment thread
claude[bot] marked this conversation as resolved.
inflight.index = i + 1;

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.
} while (!m_aboutToBeNotifiedRejectedPromises.isEmpty());
}

DEFINE_VISIT_CHILDREN(GlobalObject);
Expand Down
15 changes: 15 additions & 0 deletions src/jsc/bindings/ZigGlobalObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,21 @@ class GlobalObject : public Bun::GlobalScope {
WebCore::SubtleCrypto* m_subtleCrypto = nullptr;

Bun::WriteBarrierList<JSC::JSPromise> m_aboutToBeNotifiedRejectedPromises;

public:
// While handleRejectedPromises() is iterating its drained snapshot, this
// points at the not-yet-processed tail so promiseRejectionTracker(Handle)
// can suppress a spurious 'rejectionHandled' for a promise whose
// 'unhandledRejection' has not fired yet. Linked through `outer` to handle
// re-entrant handleRejectedPromises() calls.
struct InFlightRejections {
JSC::MarkedArgumentBuffer* buffer;
size_t index;
InFlightRejections* outer;
};

private:
InFlightRejections* m_rejectedPromisesBeingProcessed { nullptr };
};

class EvalGlobalObject : public GlobalObject {
Expand Down
57 changes: 57 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,63 @@
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.

Check warning on line 813 in test/js/node/process/process.test.js

View check run for this annotation

Claude / Claude Code Review

Test comment still references deleted takeFirst()

🟡 nit: same as the (resolved) cleanup at the C++ sites — "(instead of takeFirst() per element)" references a method this PR deletes, so a future reader has no referent. Drop the first sentence; "This test pins the observable behaviour: …" stands on its own.
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");

// A handler that .catch()es a *later* still-pending rejection must
// suppress both 'unhandledRejection' AND 'rejectionHandled' for it.
let spuriousRejectionHandled = 0;
let lateUnhandled = false;
process.on("rejectionHandled", () => spuriousRejectionHandled++);
let pLate;
process.removeAllListeners("unhandledRejection");
process.on("unhandledRejection", reason => {
if (reason === "early") pLate.catch(() => {});
if (reason === "late") lateUnhandled = true;
});
Promise.reject("early");
pLate = Promise.reject("late");
await new Promise(r => setImmediate(r));
await new Promise(r => setImmediate(r));
if (lateUnhandled) throw new Error("late promise got unhandledRejection");
if (spuriousRejectionHandled !== 0)
throw new Error("spurious rejectionHandled fired " + spuriousRejectionHandled + "x");
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