fix(promise): capture stacks on an Error for Bun - #4524
Conversation
Bun's Error.prepareStackTrace rejects a plain object holder, which throws while rewriting promise wrapper stacks. Capture on a real Error so Node and Bun share the same path. Fixes sidorares#4480
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4524 +/- ##
=======================================
Coverage 92.53% 92.53%
=======================================
Files 93 93
Lines 15995 15997 +2
Branches 2308 2308
=======================================
+ Hits 14801 14803 +2
Misses 1194 1194
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I can't reproduce original issue: |
|
You're right, a bare // Bun 1.4.0. mysql2 3.24.3 / master. No MySQL needed.
const old = Error.prepareStackTrace
Error.prepareStackTrace = (err, trace) => old(err, trace)
const holder = {}
Error.captureStackTrace(holder)
console.log(holder.stack)On Bun that throws |
|
@sidorares — here is the reproduction, driven through Why the REPL could not show it
The trigger is narrower than "Bun requires an Error"The inline comment in the diff says a plain object "works in V8, but throws there". Measured,
Both runtimes ship a non-undefined default. Neither is V8's Counter-check, because that claim is falsifiable: a non-delegating formatter The throw is eager, and that changes the blast radiusI expected the throw at the
The matrix2×2×2×2: runtime × holder × formatter × outcome.
2 of 16 cells break, both of them Scope
No behaviour change on Node that I can see: the rewritten stack is frame-for-frame identical One thing the PR does not change, offered only as an observationEven with the fix, the captured stack is much shallower on Bun than on Node. Three nested
Bun's frames also carry no function names ( The new test — one caveat and one suggestionI could not get I am explicitly not claiming this breaks CI, and the evidence is against that reading. The suggestion is small and independent of my environment: an assertion on the test file's Worth noting about that first test, though: on Node it never exercises the reported What I did not measureNo real MySQL server — the success path is |
|
Thanks for the repro @peppe1337 , I'll take a look |
|
@sidorares — I measured the performance point, since it decides this PR and nobody had a number for it. Setup, and the limits first. Node v22.22.3, Bun 1.4.0, 2-core Linux box, 1.
|
| Node 22 | ns/call |
|---|---|
new Error() alone, no capture |
4 350 |
Error.captureStackTrace({}) — what the helper does today |
4 880 |
| control: holder with no capture at all | 14 |
new Error() on its own is not more expensive than the capture the helper already performs — median 11 % below it, distributions overlapping. The 14 ns control is there so the ~5 µs figures are believable at all: the harness measures the callee, not itself.
2. What the PR actually costs is a double capture
| Node 22, ns/call | none | delegating prepareStackTrace |
|---|---|---|
{} (master) |
4 880 | 4 872 |
new Error() (this PR) |
9 973 (2.04×) | 9 746 (2.00×) |
4 350 + 4 880 = 9 230 ≈ 9 973 measured. The costs are additive: new Error() collects frames, then Error.captureStackTrace collects them again. So your instinct that this PR costs something is right, but the cause isn't that new Error() is slow — it's that the frames get gathered twice.
3. That part is removable, if you want it
function captureStackHolder(constructorOpt) {
const limit = Error.stackTraceLimit;
Error.stackTraceLimit = 0; // construction must not collect frames itself
const holder = new Error();
Error.stackTraceLimit = limit;
Error.captureStackTrace(holder, constructorOpt);
return holder;
}5 808 ns = 1.19× instead of 2.04× — that removes 82 % of the added cost.
I checked it is a real drop-in and not just a faster one: with line/column numbers normalised, the resulting frame sequence is identical to master's on node/none and node/delegating, and constructorOpt still trims the frame it is supposed to trim. On bun/delegating it behaves like the PR (master throws there — that's #4480). On Bun I would not read much into the frame comparison: my test context there was too shallow to produce visible frames, so the equality is trivially true. Where Bun frames are visible — driving the repo's own captureStackHolder + applyCapturedStack against a real error — master, the PR and this variant all yield 2 frames and all of them add a frame rather than dropping any.
Honest caveats, because it is a global mutation: nothing between the two assignments can throw, so the limit cannot leak — but it is a process-global write, and it buys nothing on Bun (654 vs 655 ns without a custom prepareStackTrace, 675 vs 688 with one). It's Node-only value for two extra property writes and a line of subtlety. Your call whether that trade is worth making.
4. On Bun this PR is a speed-up, not a cost
This is the part I did not see coming:
| Bun 1.4.0, ns/call | none | delegating |
|---|---|---|
{} (master) |
2 050 | throws |
new Error() (this PR) |
655 (0.32×) | 688 |
On Bun, Error.captureStackTrace on a plain object is 3.1× more expensive than on a real Error. End-to-end against the fake server, 16 paired rounds × 3 × 1 000 queries, comparing each variant only against the plain run of the same round: −6.5 µs/query on a 95.4 µs baseline (sign test +2/−14, p = 0.004).
5. What I cannot tell you: the end-to-end number on Node
Same harness, same 16 paired rounds: PR +10.5 µs/query on a 165.3 µs baseline, +12/−4, p = 0.077.
Do not use that number. I also ran a red test — a variant with Error.captureStackTrace removed entirely — and on Node it came out at −1.8 µs, +7/−9, p = 0.804. A harness that cannot detect the complete removal of the thing under test cannot be trusted to measure its doubling. The +10.5 µs happens to agree in direction and magnitude with the 5.1 µs from the microbenchmark, but that is agreement, not evidence. On Bun the same red test does resolve (−9.8 µs, p = 0.021), which is why I quote the Bun end-to-end figure and not the Node one.
(An earlier version of this harness reported ~41 ms/query on Node. That was delayed-ACK — the fake server had no setNoDelay. On a 10-query probe, adding it on both sides took the median from 41 ms to 0.8 ms; the 165 µs baseline above is that same path after warm-up and with the fake server's packet-sequence resync damped.)
6. Two smaller things you may want
Object.create(Error.prototype) does not work. It throws the same TypeError: First argument must be an Error object on Bun — the check is on the internal state, not the prototype. A genuine Error instance really is required, so the PR's approach is the only one available; only its execution was costly.
Frequency, measured with a counter in the source file (a patch on module.exports cannot work here — the callers destructure at require time, and mine silently reported 0 until I noticed): 1 call per query(), 1 per connection setup, 0 with trace: false. 16 is the number of call sites, not calls per query.
Whether 5 µs per query matters is your judgement, not mine — I don't know your users' stack depths, and on deep async stacks all of these numbers grow. What I think the measurements do settle: the objection is real on Node but is caused by the double capture rather than by new Error(), it is 82 % removable with the snippet above, and on Bun the change is a net win either way.
Raw data, harnesses and the pre-registered predictions (6 of 8 held, one partly, one failed — the end-to-end prediction) are reproducible from the description above; happy to post the scripts if useful.
4 participants
Thanks for the report and the pointer at capture_local_err.js. You were right: the promise wrappers were capturing the async stack on a plain object, and Bun's prepareStackTrace rejects that with "First argument must be an Error object".
This captures on a real Error instead, then still copies the frames onto the original MySQL error so codes and messages stay intact. I added a unit test that installs the same prepareStackTrace check Bun uses.
Fixes #4480