ReviewOS

zig-utils/zig-js

Threads: synchronize Promise combinator aggregation state

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

Parent: #12, #1. Related: #17.

Goal

Make Promise.all, Promise.allSettled, Promise.any, Promise.allKeyed, and Promise.allSettledKeyed aggregation records safe under shared-realm no-GIL settlement from multiple agents.

Why this is separate from #17

Issue #17 tracks direct Object.elements side doors. The result-array element writes are being migrated to locked element helpers, but the combinator aggregation record itself still has mutable shared fields:

  • promise.Combine.remaining
  • per-element already records shared by resolve/reject closures
  • final-settlement ordering / first-wins behavior across racing reactions

Those need their own synchronization story, because locking only the result array does not make the aggregation record itself race-free.

Acceptance criteria

  • Add a lock, atomics, or equivalent synchronization around combinator aggregation state.
  • Preserve Promise combinator semantics: per-element resolve/reject closure runs at most once; first rejection/fulfillment wins where specified; final settlement fires exactly once.
  • Do not hold an aggregation lock across user JS callbacks or capability resolve/reject calls; compute/publish state under the lock, then call out after releasing it.
  • Add focused parallel/no-GIL or TSan coverage for concurrently settling inputs.
  • Update threading docs with the final ruling.
9 days ago

Landed the first implementation chunk for this tracker.

Commit: e3a51b4 (fix(promise): lock combinator aggregation)

What changed:

  • Added promise.Combine.lock and a settled bit.
  • Guarded per-element already, remaining, and final/early-settlement decisions with the combine lock.
  • Early first-wins cases (Promise.all rejection, keyed all rejection, Promise.any fulfillment) now decide under the lock, then call capability reject/resolve after unlocking.
  • Final settlement is likewise decided under the lock and performed after unlocking.
  • Result-array slots continue to use the locked element helpers from the preceding #17 chunk.
  • Updated the thread audit docs with the promise.Combine ruling.

Validation:

  • zig build test -Dtest-filter="Promise" --summary none
  • git diff --check

Remaining before closing this issue:

  • Add/confirm focused parallel/no-GIL or TSan coverage for concurrently settling combinator inputs.
9 days ago

Added the focused parallel/no-GIL coverage required by this tracker.

Commit: a9cc19a (test(promise): cover parallel combinator settlement)

What changed:

  • Added parallel_js: Promise combinators tolerate concurrent input settlement.
  • The test creates main-thread promises observed by Promise.all, then has multiple no-GIL Threads release together and resolve the inputs concurrently.
  • It verifies the combined result sees every value exactly once and in aggregate (seen === 4 && sum === 10).

Validation:

  • zig build test -Dtest-filter="parallel_js: Promise combinators tolerate concurrent input settlement" --summary none
  • git diff --check

Together with e3a51b4, this satisfies the implementation, docs, and focused parallel coverage criteria for this issue.

9 days ago

Implemented in e3a51b4 and covered by a9cc19a. Closing.

Sign in to comment on this issue.