-
-
Notifications
You must be signed in to change notification settings - Fork 159
fix(gc): root heap values held across JS-running calls in runtime and ext-http #8131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
28f63bf
fix(runtime): root the generic array-like callback loops across their…
0aeb25c
fix(runtime): root call/apply and put-value locals across their JS in…
5ad41b2
fix(ffi): transient GC roots for ext-crate callback snapshots
86d034b
feat(gc): sharpen the from-space scan and stack-map walk instruments
1891d32
test(eh): pin that an action-zero landing pad is still Perry's catch
00d6b36
fix(ext-http): make the GC scanner tests force evacuation, and drop t…
444c60f
docs: changeset for the moving-GC rooting sweep
695d44b
refactor(gc): use the sanctioned handle accessors in the arraylike loops
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| ### Fixed | ||
|
|
||
| - Root heap values held in Rust locals across calls that can run JS and | ||
| therefore collect. Each was caught by a fault at the exact instruction | ||
| under `PERRY_GC_PROTECT_FROMSPACE=1`, not by inspection: | ||
| - the generic array-like callback helpers (`forEach`, `map`, `filter`, | ||
| `some`, `every`, `find`/`findIndex`/`findLast`/`findLastIndex`, | ||
| `reduce`/`reduceRight`) held the receiver, callback, result under | ||
| construction, and current element across `js_closure_call*`; `map` | ||
| wrote every mapped element through a pre-collection element pointer, | ||
| landing in retired from-space; | ||
| - `Function.prototype.call`/`.apply` held the callee, the explicit | ||
| `this`, and the saved implicit-`this` across the invocation, then read | ||
| the stale callee's header in the native-this alias check; | ||
| - `js_put_value_set` held the receiver and property key across | ||
| `ordinary_set_with_receiver` (which runs user setters) before the | ||
| array-subclass `length` note dereferenced them. | ||
|
|
||
| - Keep perry-ext-http's listener dispatch on values the collector can see. | ||
| Ext handle-struct side tables are rewritten by registered scanners, but a | ||
| SNAPSHOT of one in a Rust local is a copy no scanner reaches: a drained | ||
| listener vec went stale after the first callback's collection, and the | ||
| pending-request struct parked in an mpsc channel went stale across the | ||
| microtask-pump safepoint minors that run while the request waits. The | ||
| emit helpers, deferred-listen drain and close callback now root their | ||
| snapshots, and both request dispatchers re-read handler and listener | ||
| lists from the scanner-maintained server handle at dispatch time. The | ||
| orphaned `HttpPendingRequest::check_continue_listeners` field is removed; | ||
| only the routing bit crosses the channel. | ||
|
|
||
| - Repair two GC scanner tests that were failing on `main`: they assert a | ||
| root was rewritten, which is only observable if the collection actually | ||
| moved the object, and evacuation is a C4b policy decision that | ||
| legitimately declines under unit-test conditions. Their guards now force | ||
| evacuation for their mutex-serialized lifetime. | ||
|
|
||
| ### Added | ||
|
|
||
| - `perry_ffi::TransientRootScope` — a safe wrapper over a new extern | ||
| surface onto the runtime's transient-handle stack, so ext crates can root | ||
| the table snapshots they hold across JS callbacks. | ||
|
|
||
| - Instruments: the whole-heap from-space scan appends a classified payload | ||
| preview to each offender, so the owner identifies itself instead of being | ||
| an anonymous address; `PERRY_GC_STACKMAP_TRACE=1` prints every frame the | ||
| native stack-map walk visits; `PERRY_EH_TRACE=1` prints per-frame | ||
| personality decisions. | ||
|
|
||
| ### Testing | ||
|
|
||
| - A deterministic moving-GC regression for `js_arraylike_map` (a callback | ||
| that runs a copying minor on every invocation), sabotage-verified. | ||
| - `action_zero_pad_is_still_a_handler` pins that a zero call-site action is | ||
| still Perry's catch — the shape of every JS `try` under native roots | ||
| (#7982) — so reading the action as "handler vs cleanup" can no longer | ||
| silently disable every statepoint-built catch. Sabotage-verified. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not mutate this process-global GC setting with separate locks.
GC_TEST_LOCKis a different static in each module. Parallel tests can therefore overlap these guards. One guard can removePERRY_GC_FORCE_EVACUATEwhile another guard still requires it. The unsafe safety claims do not exclude other test modules or runtime threads that read the environment.Replace the environment mutation with a runtime test override that is safe for concurrent readers. Preserve and restore the prior setting if an environment fallback remains necessary.
crates/perry-ext-http/src/server/mod.rs#L197-L206: use the shared runtime test override instead ofset_var.crates/perry-ext-http/src/server/mod.rs#L217-L218: release the shared override without unconditionally removing inherited environment state.crates/perry-ext-http/src/tests.rs#L18-L32: use the same shared runtime test override.crates/perry-ext-http/src/tests.rs#L43-L44: release the shared override without unconditionally removing inherited environment state.📍 Affects 2 files
crates/perry-ext-http/src/server/mod.rs#L197-L206(this comment)crates/perry-ext-http/src/server/mod.rs#L217-L218crates/perry-ext-http/src/tests.rs#L18-L32crates/perry-ext-http/src/tests.rs#L43-L44🤖 Prompt for AI Agents