ReviewOS

zig-utils/zig-js

Threads: synchronize Promise combinator aggregation state

#19
Closed chris opened this 10 days ago ·
10 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.
10 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.
10 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.

10 days ago

Implemented in e3a51b4 and covered by a9cc19a. Closing.

Sign in to comment on this issue.