ReviewOS

zig-utils/zig-js

Threads: serialize shared Map and Set iterator cursor advancement

#643
Closed chris opened this 9 days ago ·
9 days ago

Problem

Fresh required TSan CI reports three races in the shared Map iterator cursor witness: concurrent next() calls copy the iterator cursor's three-word CollectionIteratorState in Object.collectionIteratorState while peer threads update index, current, and traversal state in collectionCursorIterNext.

The semantic test happens to finish successfully, but these are engine-state races. A multiword cursor snapshot can be torn even when individual word reads/writes appear valid, violating the repository's no-GIL synchronization contract.

Required implementation

  • Give each published collection iterator cursor an explicit synchronization ruling.
  • Make state inspection and advancement one atomic logical transaction without serializing unrelated Map/Set instances or the underlying collection's independent readers.
  • Preserve iterator ordering, deletion/insertion semantics, exhausted-state idempotence, and GC tracing/relocation of the cursor's managed edges.
  • Do not suppress TSan, make the fields independently atomic, restore the context GIL, or weaken the shared-cursor witness.

Acceptance

  • parallel_js: Map iterator cursor advances atomically without GIL passes with zero TSan reports.
  • Focused tests cover concurrent next(), exhaustion, mutation during iteration, Map keys/values/entries, Set values/entries, and GC while a cursor is shared.
  • Normal, TSan, no-GIL, and Test262 Map/Set iterator coverage remain green.

Parent: #478 Related CI: tsan-parallel-js

9 days ago

Fresh Linux suppression-free TSan still reproduces the cursor race on current main: tsan-parallel-js job 96534732523. The new report is at direct writes in collectionCursorIterNext (interpreter.zig lines 34513, 34523, and 34524), with 3 TSan warnings; the functional test itself returns OK and the remaining 61-case focused gate completes 54 pass / 8 skip / 0 fail / 0 leak. I am leaving implementation ownership with the user as requested.

9 days ago

The #643 witness recurred unchanged in workflow 32423835718 for unrelated JSON commit 0052b2aa: tsan-parallel-js reported 2 races and TSan unit shard 1 reported 3, all in the Map iterator cursor path (value.Object.collectionIteratorState / interpreter.collectionCursorIterNext). The other 49 jobs succeeded and 1 skipped. No iterator fix was attempted here; this is linked CI evidence for the owner.

9 days ago

Fresh descendant CI evidence from cancelled/superseded run 32488007100 reproduces this exact cursor race in both sanitizer gates:

  • tsan-unit (shard 3/4): context.test.parallel_js: Map iterator cursor advances atomically without GIL, two TSan warnings; both summaries point through value.Object.collectionIteratorState at src/value.zig:3090 and __tsan_memcpy.
  • tsan-parallel-js: the same witness, one TSan warning and the same stack boundary.

Functional assertions still completed (415 passed / 32 skipped / 0 failed in the shard; 54 / 8 / 0 in focused parallel_js), but TSan correctly exited 66. I made no Map/Set iterator, stack, queue, Promise, microtask, Worker, or job changes; this is evidence only for the user-owned #643 fix.

9 days ago

Landed in 04d46c59 (`fix(collections): make shared cursor dispatch one locked transaction (#643)`).

Synchronization ruling (recorded on the advance helper): a published collection-iterator cursor is guarded by its owning iterator object's `property_lock`; inspection and advancement are one atomic logical transaction inside that critical section; concurrent GC trace reads `source` through the backing lock and relocation rewrites it only at the world-stopped commit.

The fix removes the unlocked rare-state probe that Linux TSan flagged: dispatch first runs a content-free atomic tag check (the rare tag is immutable once published, so non-collection iterators — the hot array path — never acquire a lock), then probes and advances under the same property lock that serializes every cursor-field store. No unrelated Map/Set instance or the collection's own readers are serialized; per-entry exactly-once semantics, exhaustion idempotence, insertion-order/

9 days ago

Linux CI confirmation for the landing commit 04d46c59: run 32592838252 passes `tsan-unit` (all four shards) and `tsan-parallel-js` with zero cursor-race diagnostics — the same gates that previously reported the `collectionIteratorState`/`__tsan_memcpy` race on every run since 32356955396. The run's only failure is the pre-existing `semantics/oom-one-thread.js`, now tracked in #722.

Sign in to comment on this issue.