Buffer emissions for async iterable integrity in Poll#792
Open
afshin wants to merge 3 commits into
Open
Conversation
06f4f6b to
b0acee5
Compare
b0acee5 to
6cfd134
Compare
Poll
There was a problem hiding this comment.
Pull request overview
Updates Poll#[Symbol.asyncIterator] to buffer ticked emissions per iterator, preventing rapid schedule() calls from overwriting intermediate states before a consumer resumes iteration.
Changes:
- Reworked
Poll#[Symbol.asyncIterator]to enqueue eachtickedstate and yield in FIFO order, withfinally-based disconnect cleanup. - Added tests validating no dropped states under rapid
schedule()calls, correct behavior after disposal, and unchanged behavior for one-tick-at-a-time polling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/polling/src/poll.ts | Implements per-iterator buffering of ticked emissions and ensures signal disconnection on iterator termination. |
| packages/polling/tests/src/poll.spec.ts | Adds regression and behavior tests covering rapid scheduling, disposal, and baseline iteration behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f13d84d to
b695c17
Compare
b695c17 to
9083b0c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates
Poll#[Symbol.asyncIterator]with a buffered implementation that connects to thetickedsignal for each iterator returned to a client, ensuring rapidschedule()calls no longer silently drop intermediate states.Previously, the iterator read
this.statedirectly on each yield. Whenschedule()was called multiple times before the consumer resumed,this._statewas overwritten and only the last state was visible.The implementation in this PR pushes every
tickedemission into a per-iterator queue and yields from it in order. When the queue is empty, it awaitsthis.tickas a wake-up signal. Cleanup is handled in afinallyblock to guarantee the signal listener is disconnected onbreak,return,throw, or disposal.This problem does not exist in
Streamfrom@lumino/signalingbecause stream's async iterator is based off of a promise chain instead of being a push-based state machine likePoll. Theyieldparadigm is pull-based. So to bridge this gap naively like the original implementation inPollmeant silently dropping emissions that disappear with no error because the pollstateis being overwritten before the nextyield. Effectively, then,Streamhas a functionally unbound queue (a linked list of promises) andPollhas a bound queue of 1000 emissions that can pile up if a client consumes yielded values slowly.No new API surface, no type changes, no effect on
Throttler/RateLimiter.Normal single-tick-at-a-time polling behaves identically.
The added tests in the polling test suite will fail (timeout) on the
mainbranch but will pass with these changes.