ReviewOS

zig-utils/zig-js

Threads: synchronize DisposableStack resource records

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

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

Goal

Give DisposableStack and AsyncDisposableStack resource records a no-GIL synchronization story for same-stack cross-thread calls.

Current state

The resource list is stored as an internal element-backed array (\x00ds_res) containing internal entry tuples [value, onDispose, kind]. The element-storage side can be moved behind Object.elements_lock / snapshots, but the stack also has observable mutable state:

  • \x00ds_done
  • \x00ds_res transfer during move()
  • push/use/adopt/defer versus dispose/disposeAsync
  • reverse disposal iteration while user callbacks run

A same-stack call from another thread can race a use/adopt/defer append with dispose/disposeAsync setting disposed, snapshotting/clearing resources, and invoking callbacks.

Acceptance criteria

  • Decide/document whether DisposableStack objects are shared-realm callable from multiple JS threads or thread-affine.
  • If shared, add a lock or equivalent synchronization around disposed-state publication, resource-list transfer, push, and disposal snapshot/clear.
  • Do not hold the stack/resource lock across user disposal callbacks or awaited async disposal.
  • Preserve reverse-disposal order and SuppressedError chaining.
  • Add focused no-GIL coverage for concurrent same-stack push/dispose behavior.
  • Update docs/threads/bindings.md with the final ruling.
9 days ago

Created from the #17 DisposableStack element-storage audit.

Commit 4ba7d5d (fix(dispose): snapshot resource entries) closed the direct element-list side of this:

  • resource tuple/list appends use internal locked helpers,
  • disposal snapshots the resource list before callbacks / awaits,
  • tuple fields read through elementAt().

Remaining for this issue is the broader same-stack synchronization policy: disposed-state publication, move/resource-list transfer, and push vs dispose races need a shared lock or documented thread-affinity ruling. That should not be hidden under #17 because it is not just an Object.elements side door.

9 days ago

Implemented and validated.

Commit: 63f29ff (fix(dispose): lock stack resource state)

What changed:

  • Uses the stack object's elements_lock as the native state mutex for disposed-state publication, resource-list push, move() transfer, and dispose/disposeAsync snapshot+clear.
  • dispPush rechecks disposed under that mutex before appending the resource entry, so same-stack push racing with dispose observes ReferenceError.
  • dispose / disposeAsync atomically snapshot and clear resources while publishing disposed, then release the lock before user disposal callbacks or async awaits.
  • move() transfers the source resource list and marks the source disposed under the same mutex.
  • disposed getter reads under the state mutex.
  • Added a no-GIL witness where a worker blocks inside a disposal callback while the main thread attempts defer; the main thread gets ReferenceError and does not deadlock, proving the lock is not held across the callback.
  • Updated docs/threads/bindings.md with the final shared-stack ruling.

Validation:

  • zig build test -Dtest-filter='parallel_js: DisposableStack rejects concurrent push during dispose callback' --summary none
  • zig build test -Dtest-filter='using' --summary none
  • zig build test -Dtest-filter='SuppressedError' --summary none
  • git diff --check
9 days ago

Closing: DisposableStack / AsyncDisposableStack resource state is now serialized in 63f29ff, with no-GIL coverage and docs updated.

Sign in to comment on this issue.