feat(phaser): add reusable phase coordination - #250
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Phaser::poll_wait currently clones wakers unconditionally instead of following the repo’s established WaitSet::will_wake pattern, adding avoidable per-poll overhead.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new opt-in asyncband::phaser synchronization primitive for coordinating repeated phases with a dynamic participant set, including RAII participants, cancel-safe waiting semantics, and documentation/tests to integrate it into the crate’s public surface.
Changes:
- Introduce
asyncband::phaser::{Phase, Phaser, PhaserParticipant}behind a newphaserfeature flag. - Add unit + integration tests covering registration/arrival/advance semantics and cross-task waiting.
- Update crate docs (README + crate-level docs) and changelog to advertise the new primitive.
File summaries
| File | Description |
|---|---|
| tests-integration/tests/traits_test.rs | Extends trait assertions (Send/Sync/Unpin) to cover new public phaser types. |
| tests-integration/tests/phaser_test.rs | Adds integration tests for spawned-task waiting and observer semantics. |
| tests-integration/Cargo.toml | Enables asyncband’s new phaser feature for integration tests. |
| README.md | Documents the new Phaser feature in the public feature table. |
| CHANGELOG.md | Records the addition of the new opt-in Phaser. |
| Cargo.lock | Captures new dev/test dependency resolution (e.g., tokio-test). |
| asyncband/src/phaser/tests.rs | Adds focused unit tests for phase advancement, cancellation, waker behavior, and wraparound. |
| asyncband/src/phaser/mod.rs | Implements the new Phaser primitive, participants, waiting, and internal state transitions. |
| asyncband/src/lib.rs | Wires the phaser module into the crate behind a feature flag and updates crate docs. |
| asyncband/src/internal/mod.rs | Extends internal module feature gating to include phaser where needed. |
| asyncband/Cargo.toml | Adds the phaser feature and includes tokio-test for module tests. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fn poll_wait( | ||
| &self, | ||
| token: &mut Option<WakerToken>, | ||
| observed: Phase, | ||
| cx: &mut Context<'_>, | ||
| ) -> Poll<Phase> { | ||
| let waker = cx.waker().clone(); | ||
| let _retired_waker = { | ||
| let mut state = self.state.lock(); | ||
| if state.phase != observed { | ||
| let phase = state.phase; | ||
| *token = None; | ||
| return Poll::Ready(phase); | ||
| } | ||
| state.waiters.register(token, waker) | ||
| }; | ||
| Poll::Pending | ||
| } |
orthur2
left a comment
orthur2
left a comment
There was a problem hiding this comment.
Could you rebase this onto current main branch first? Since the branch was last updated, #273 replaced WaitSet with WakerSet, and the recent API-table changes now conflict with this branch.
I'd also recommend adding a short Summary to the PR description, in line with AGENTS.md.You can refer to the body of PRs that have already been merged.
I'd be happy to do a more thorough review once it's rebased.
|
Updated. |
| * Add opt-in `asyncband::once::LazyCell` for values that own one asynchronous initializer and preserve its in-flight future across caller cancellation. | ||
| * Add opt-in bounded and unbounded runtime-agnostic object pools under `asyncband::pool`. | ||
| * Add an opt-in `asyncband::blocking::FutureExt` bridge with `block_on` and `wait_timeout` methods for waiting on runtime-agnostic futures from synchronous code. | ||
| * Add an opt-in runtime-agnostic `Phaser` with dynamic RAII participants, reusable phases, and cancellation-resilient arrival semantics. |
There was a problem hiding this comment.
Could you move this entry to Unreleased list?
| //! | ||
| //! Waiters compare phase identity instead of inferring transitions from party counts. | ||
| //! | ||
| //! # Examples |
There was a problem hiding this comment.
Could we add an async example showing a few phases, including separate arrival and waiting?
There was a problem hiding this comment.
Currently, a spawned task needs a separate Arc<Phaser> to call wait_for_advance() after participant.arrive(). The participant already holds that same Arc internally. I'd suggest exposing a phaser() accessor so callers can use the two operations separately without carrying another handle.
| //! | ||
| //! [`Phaser::arrived_parties`] is the difference between the registered and unarrived counts. | ||
| //! | ||
| //! All state transitions and waiter registration share one synchronization point. |
There was a problem hiding this comment.
Could we spell out the memory visibility guarantee here?
The shared synchronization point explains how the internal transitions are ordered, but does not explicitly tell callers whether operations before each participant's arrival happen-before operations after a wait observes that phase's completion. The shared mutex already provides this guarantee, but it should be explicit in the public API docs.
I added a # Synchronization section for this in ManualResetEvent last week. Something similar would help here.
Summary
Phasersynchronization primitive with dynamically registered RAII participants.Design Notes
Registration returns a
PhaserParticipantcapability instead of exposing counter-oriented arrival methods directly. This associates each registration with a participant lifecycle and prevents duplicate arrival within one phase.Dropping a participant arrives and deregisters it. Cancelling
arrive_and_waitafter its first poll does not retract the committed arrival, and retrying waits for the previously recorded phase instead of arriving again.An empty phaser remains dormant and can accept new participants later. Waiters identify completion by phase identity, and phase values use wrapping equality without providing a total ordering across wraparound.