Skip to content

fix(thread): worker gc-init, rooted rebuilt closures, cross-thread promises in malloc space, ws deferred resolution#6188

Merged
proggeramlug merged 2 commits into
mainfrom
fix/gc-w1-threading
Jul 9, 2026
Merged

fix(thread): worker gc-init, rooted rebuilt closures, cross-thread promises in malloc space, ws deferred resolution#6188
proggeramlug merged 2 commits into
mainfrom
fix/gc-w1-threading

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Wave 1 of the 2026-07-09 GC audit (fable-audit-perry-gc.md), threading dimension — the quick, mechanical subset. The structural cross-heap questions (owner-tagged queues, module globals in thread closures) are tracked in #6185; nothing here changes the threading model.

1. Worker threads could collect with zero registered root scanners (P1)

MUTABLE_ROOT_SCANNERS is thread-local and ensure_gc_initialized was wired only through js_get_global_this. A spawn/parallelMap/parallelFilter worker whose closure never reads a global but allocates past a trigger (e.g. deserializing large captures/elements) ran a collection with an empty scanner registry and an empty shadow stack — sweeping everything it had just built (singleton-closure caches, boxes, class side tables all invisible). Every worker entry now calls ensure_gc_initialized() before its first allocation. It is idempotent and cheap.

2. Rebuilt worker closures were unrooted across deserialization (P1)

The worker-side local_closure was a bare Rust local; every deserialize_nanbox_on_current_thread between call_fn invocations can allocate and trigger a worker GC, sweeping the closure and its captures mid-loop. The rebuilt closure is now rooted in a RuntimeHandleScope and re-derived from the handle at each call (spawn body, parallelMap worker, parallelFilter worker).

3. Cross-thread promises moved out of the copying nursery (P1)

spawn()/Atomics.waitAsync pin a promise and hand its raw usize to the global PENDING_THREAD_RESULTS queue, which no root scanner visits. The copied-minor from-space flip resets eden/survivor blocks wholesale — only root-reachable pins force the fallback — so a fire-and-forget spawn promise could be recycled while the worker still held its address; js_thread_process_pending then resolved through freed memory.

New js_promise_new_cross_thread() allocates these promises via gc_malloc: malloc space is non-moving, both sweep paths honor GC_FLAG_PINNED, and (since #6178) old→malloc edges are remembered. Used by spawn_impl and js_atomics_wait_async; ordinary promises are untouched.

4. ws.waitForMessage resolved with a tokio-arena string (P1)

js_ws_wait_for_message allocated the result StringHeader on the tokio worker's thread-local arena and queued the raw pointer for main-thread resolution — a cross-heap pointer freed under the main thread by the worker's GC — and tagged it POINTER_TAG, so the awaited value was a string-like object (typeof === "object"). Converted to the #1292 queue_deferred_resolution pattern (bcrypt.rs): the owned bytes move into the converter closure, the JS string is built on the main thread, tagged STRING_TAG. This was the only spawn_for_promise discipline violation found in perry-stdlib.

5. single_thread_map/single_thread_filter cached elements_ptr across user calls (P3)

The input array's elements pointer was computed once and re-read after each user callback (which can trigger a moving minor). The input array is now rooted before the result allocation and the elements pointer re-derived from the handle per iteration, matching the array iter_methods.rs discipline.

Tests

Full perry-runtime lib suite: 1164 passed / 0 failed; perry-stdlib compiles clean. cargo fmt --check clean.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved thread-safety during concurrent operations by properly initializing GC state in worker threads.
    • Fixed memory allocation for cross-thread promises to prevent garbage collection scanning issues.
  • Performance

    • Optimized promise allocation strategy for async operations and parallel tasks.
    • Enhanced WebSocket message resolution handling.

Ralph Küpper added 2 commits July 9, 2026 12:54
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8490c1f4-576a-491d-82d3-f841514a284c

📥 Commits

Reviewing files that changed from the base of the PR and between da02744 and 9c8bcf0.

📒 Files selected for processing (5)
  • crates/perry-runtime/src/atomics.rs
  • crates/perry-runtime/src/promise/mod.rs
  • crates/perry-runtime/src/promise/then.rs
  • crates/perry-runtime/src/thread.rs
  • crates/perry-stdlib/src/ws.rs

📝 Walkthrough

Walkthrough

This PR introduces a cross-thread promise allocation path that forces malloc-space allocation instead of nursery allocation, applies it to atomics wait and spawn operations, and adds GC initialization plus handle-scope rooting across parallelMap/parallelFilter/spawn worker threads. It also defers websocket message string construction to the main thread.

Changes

Cross-thread GC Safety and Promise Allocation

Layer / File(s) Summary
Cross-thread promise allocation primitive
crates/perry-runtime/src/promise/then.rs, crates/perry-runtime/src/promise/mod.rs
Adds force_malloc flag to js_promise_new_with_parent_impl and new js_promise_new_cross_thread entry point that forces GC malloc allocation for cross-thread promises; reformats the then module re-export list.
Atomics and spawn adopt cross-thread promise
crates/perry-runtime/src/atomics.rs, crates/perry-runtime/src/thread.rs
js_atomics_wait_async and spawn_impl switch from js_promise_new() to js_promise_new_cross_thread() for background/OS-thread promises.
parallelMap worker GC init and rooting
crates/perry-runtime/src/thread.rs
Worker threads call ensure_gc_initialized() and root the reconstructed closure in a RuntimeHandleScope; single_thread_map roots the input array before allocating the result array.
parallelFilter worker GC init and rooting
crates/perry-runtime/src/thread.rs
Worker threads call ensure_gc_initialized() and root the reconstructed closure; single_thread_filter roots the input array before allocating the result array.
spawn thread GC init and rooting
crates/perry-runtime/src/thread.rs
The spawned OS thread calls ensure_gc_initialized() and reconstructs its closure under a rooted RuntimeHandleScope.
Deferred websocket string resolution
crates/perry-stdlib/src/ws.rs
js_ws_wait_for_message defers StringHeader construction to the main thread via queue_deferred_resolution and resolves using JSValue::string_ptr(...) instead of building the string on the tokio worker.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TokioWorker
  participant DeferredQueue
  participant MainThread
  participant Promise
  TokioWorker->>DeferredQueue: queue_deferred_resolution(message)
  DeferredQueue->>MainThread: dispatch closure
  MainThread->>MainThread: construct StringHeader
  MainThread->>Promise: resolve with JSValue::string_ptr(...).bits()
Loading

Possibly related issues

Possibly related PRs

  • PerryTS/perry#5188: Both PRs modify js_promise_new_with_parent/allocation logic in crates/perry-runtime/src/promise/then.rs, overlapping on promise allocation behavior.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main threading and GC-safety changes in the PR.
Description check ✅ Passed The description covers the summary, concrete changes, related issue, and test results, with only non-critical template sections omitted.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/gc-w1-threading

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant