Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReadableStream @@asyncIterator #15097

Merged

Conversation

MattiasBuelens
Copy link
Contributor

@MattiasBuelens MattiasBuelens commented Jan 26, 2019

Add tests for async-iterating a ReadableStream. See whatwg/streams#980 for the accompanying spec change.

This PR continues from @devsnek's work in #13362.

The tests are based on Domenic's comment in whatwg/streams#954. So far, the following tests have been implemented:

  • Basic test of a stream with a few chunks that closes
    • Using a "push" underlying source (calls c.enqueue() in start) that enqueues all at once
    • Using a "push" underlying source where you call c.enqueue() "just in time" (i.e. inside the loop body)
    • Using a "pull" underlying source (calls c.enqueue() in pull) using recordingReadableStream to verify the correct underlying source method calls
  • Degenerate streams
    • Async-iterating an errored stream throws
    • Async-iterating a closed stream never executes the loop body, but works fine
    • Async-iterating an empty but not closed/errored stream never executes the loop body and stalls the async function (end the test if 50 ms pass without the promise resolving)
  • @@asyncIterator() method is === to getIterator() method
  • Cancellation behavior (use recordingReadableStream)
    • Manually calling return() causes a cancel
    • throwing inside the loop body causes a cancel
    • breaking inside the loop body causes a cancel
    • returning inside the loop body causes a cancel
    • All of the above, but with preventCancel: true and the pass conditions reversed
  • Manual manipulation
    • double-return rejects (#954 (comment))
    • next()'s fulfillment values have exactly the right shape (Object.prototype, only two properties, correct property descriptors)
    • throw method does not exist
    • Calling next() or return() on non-ReadableStreamDefaultReaderAsyncIterator instances rejects with TypeError (there are existing brand check tests you can add to)
    • Calling return() while there are pending reads (i.e. unsettled promises returned by next()) rejects the return Promise (#950 (comment))
  • Interaction with existing infrastructure
    • getIterator/@@asyncIterator throw if there's already a lock
    • Monkey-patch getReader and default reader's prototype methods and ensure this does not interfere with async iteration (e.g. using one of the basic test cases)
    • Basic test with a few chunks but you've already consumed one or two via a normal reader (which you then released)
  • Auto-release
    • You can still acquire a reader and successfully use its .closed promise after exhausting the async iterator via for-await-of
    • You can still acquire a reader and successfully use its .closed promise after return()ing from the async iterator (either manually or via break; take your pick)

Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

streams/readable-streams/async-iterator.any.js Outdated Show resolved Hide resolved
streams/readable-streams/async-iterator.any.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

We should land this at the same time as the standard changes.

@MattiasBuelens
Copy link
Contributor Author

I still want to add two more tests that were originally suggested:

  • Using a "pull" underlying source (calls c.enqueue() in pull) using recordingReadableStream to verify the correct underlying source method calls
  • throw method does not exist

I'll look into adding them.

@MattiasBuelens
Copy link
Contributor Author

MattiasBuelens commented Jan 30, 2019

Last few tests added. Now it's just a matter of fixing the reference implementation to pass them...

EDIT: Reference implementation is passing again.
For me this is ready to go (together with the spec change). 😄

@MattiasBuelens MattiasBuelens changed the title [WIP] ReadableStream @@asyncIterator ReadableStream @@asyncIterator Jan 30, 2019
@ricea
Copy link
Contributor

ricea commented Feb 4, 2019

Still lgtm with latest changes.

@ricea ricea merged commit de6f8fc into web-platform-tests:master Feb 6, 2019
@MattiasBuelens MattiasBuelens deleted the readablestream-asynciterator branch February 6, 2019 09:51
@guest271314
Copy link
Contributor

Is this behind a flag at Chromium?

@ricea
Copy link
Contributor

ricea commented Mar 24, 2020

Is this behind a flag at Chromium?

No. See https://bugs.chromium.org/p/chromium/issues/detail?id=929585.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants