fix(gc): three more re-read-vs-stale-local orderings around the async result promise (#7497 follow-up) - #7529
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe runtime re-reads rooted promise, value, and closure pointers after allocation-sensitive operations. Changelog entries document the attempted fixes and verification results. The protected auto-optimized crash remains unresolved. ChangesAsync Promise GC Pointer Stabilization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… result promise (#7497) js_async_step_done passed the pre-root copy of trap_next to the settle instead of the re-read address; resolve_trap_next_with_adoption held its receiver across js_assimilate_thenable, which runs a user then getter; and the AsyncStep arm seeded INLINE_TRAP from its locals rather than from the handles it had just re-read. All three are correct on their own terms and none of them clears the residual post-output fault in js_async_step_done under the auto-optimize link, which the changelog now says explicitly rather than implying the instrument is clean.
2c6c15a to
9ad65a0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Follow-up to #7516 (which landed as 5edfe99 while this was in flight). Same
family, three more
re-read vs stale localorderings around the async resultpromise, plus one honesty correction to #7516's changelog fragment.
The three orderings
js_async_step_donepassed the pre-root copy. fix(gc): Promise.all at scale read globalThis and its own combinator state from retired from-space (#7497) #7516 rootedtrap_nextand then handed
resolve_trap_next_with_adoptionthe original bindinganyway. Rooting is not a licence to keep using the old name — that is the
exact shape fix(gc): Promise.all at scale read globalThis and its own combinator state from retired from-space (#7497) #7516 exists to remove.
resolve_trap_next_with_adoptionheld its receiver acrossjs_assimilate_thenable. That call runs a userthengetter, andtargetis the receiver of both settlements after it. This is the one with areal user-facing path behind it (
return <thenable>from an async fn —Drizzle's
QueryPromise,await thenablereturns the thenable itself instead of calling.then(resolve, reject)#586).The
AsyncSteparm seededINLINE_TRAPfrom its locals. Between there-read at the top of the arm and the
INLINE_TRAP.setsit therunaway-reentry guard (which allocates a TypeError on its bounded path) and
two handle pushes. A stale
nextparked in the trap is whatjs_async_step_donelater settles and returns as the async function's ownresult promise.
What this does NOT do, stated plainly
#7516's fragment recorded one open residual: a protected run
(
PERRY_GC_PROTECT_FROMSPACE=1) of the auto-optimize binary prints the correctchecksum and then faults inside
js_async_step_done. All three changes abovewere attempts at it and none of them cleared it — the holder is somewhere I
have not found. The fragment is amended to say so rather than leaving the
impression that the instrument is clean or that one more re-read would do it.
Each change is nonetheless correct on its own terms and is kept for that reason,
not because it was measured to fix something.
Verification (before the merge of #7516 moved main under it; re-checked after
the rebase)
benchmarks/app-patterns/kernels/promise_all_chains.ts—checksum: 2500050000,byte-exact vs node 26.5.1, on both link modes.
scripts/auto_opt_app_patterns.sh— 12/12, no skips.test_gap_gc_global_builtin_lookup_rooting.tsplus 8 promise/array/async gaptests — all byte-exact.
cargo test -p perry-runtime— 1744 passed, 0 failed.scripts/raw_handle_debt.py999 (unchanged),cargo fmt --checkclean.Summary by CodeRabbit
Bug Fixes
Documentation
Maintenance