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

add @@asyncIterator to ReadableStream #954

Closed
wants to merge 7 commits into from

Conversation

@devsnek
Copy link

devsnek commented Sep 7, 2018

Closes #778


Preview | Diff

index.bs Outdated

This functionality is especially useful for creating abstractions that desire the ability to consume a stream in its
entirety. By getting a reader for the stream, you can ensure nobody else can interleave reads with yours or cancel
the stream, which would interfere with your abstraction.

This comment has been minimized.

Copy link
@domenic

domenic Sep 7, 2018

Member

This copy-pasted text seems not as useful as the old text.

index.bs Outdated
@@ -866,6 +868,84 @@ option. If <code><a for="underlying source">type</a></code> is set to <code>unde
</code></pre>
</div>

<!-- Bikeshed doesn't let us mark this up correctly: https://github.com/tabatkins/bikeshed/issues/1344 -->
<h5 id="rs-asynciterator" for="ReadableStream">[@@asyncIterator]({ <var>preventCancel</var> = false } = {})</h5>

This comment has been minimized.

Copy link
@domenic

domenic Sep 7, 2018

Member

What happened to getIterator()?

index.bs Outdated
lt="ReadableStreamAsyncIteratorPrototype">ReadableStreamAsyncIteratorPrototype</h3>

{{ReadableStreamAsyncIteratorPrototype}} is an ordinary object that is used by {{ReadableStream/[@@asyncIterator]()}} to
construct the objects it returns. Instances of {{ReadableStreamAsyncIteratorPrototype}} implmenet the {{AsyncIterator}}

This comment has been minimized.

Copy link
@domenic

domenic Sep 7, 2018

Member

Typo "implement" (maybe my fault).

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
exception.
1. Let _result_ be ! ReadableStreamReaderGenericCancel(_reader_, _value_).
1. Otherwise,
1. Let _result_ be ! ReadableStreamCreateReadResult(_value_, *true*, *true*).

This comment has been minimized.

Copy link
@domenic

domenic Sep 7, 2018

Member

This needs to be a promise resolved with...

index.bs Show resolved Hide resolved
devsnek added 2 commits Sep 7, 2018
index.bs Outdated
1. Otherwise,
1. Let _result_ be _value_.
1. Perform ! ReadableStreamReaderGenericRelease(_reader_).
1. Return <a>a promise resolved with</a> ! ReadableStreamCreateReadResult(_result_, *true*, *true*).

This comment has been minimized.

Copy link
@domenic

domenic Sep 7, 2018

Member

So now if preventCancel is false return() will give a promise for { value: promise for undefined, done: true }. Whereas if it is true, return() will give a promise for { value: value, done: true }. It should give the latter in both cases.

Copy link
Member

domenic left a comment

Looks good to me! Just needs reference implementation and tests. Also good to get @ricea's sign-off, of course.

Copy link
Contributor

MattiasBuelens left a comment

Still some holes left in return().

1. Perform ! ReadableStreamReaderGenericRelease(_reader_).
1. Return the result of <a>transforming</a> _result_ by a fulfillment handler that returns !
ReadableStreamCreateReadResult(_value_, *true*, *true*).
1. Perform ! ReadableStreamReaderGenericRelease(_reader_).

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Sep 7, 2018

Contributor

This should reject if reader.[[ownerReadableStream]] is undefined. Otherwise, we may fail an assert in ReadableStreamReaderGenericRelease.

We should probably just move step 3a to before step 3.

This comment has been minimized.

Copy link
@ricea

ricea Sep 10, 2018

Collaborator

I think it's impossible for it to be undefined. User code would have to get access to [[reader]] to call releaseLock(), and it can't. So this should be an assert.

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Sep 10, 2018

Contributor

It's not impossible for it to be undefined, but you really have to go out of your way to break it:

const iterator = readable.getIterator();
iterator.return(); // releases lock
iterator.return(); // boom!
1. Perform ! ReadableStreamReaderGenericRelease(_reader_).
1. Return the result of <a>transforming</a> _result_ by a fulfillment handler that returns !
ReadableStreamCreateReadResult(_value_, *true*, *true*).
1. Perform ! ReadableStreamReaderGenericRelease(_reader_).

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Sep 7, 2018

Contributor

What happens when there are pending read requests? See previous discussion.

Normally, ReadableStreamDefaultReader.releaseLock() checks whether this.[[readRequests]] is empty. However, this implementation uses ReadableStreamReaderGenericRelease directly without going through the same checks as releaseLock.

@domenic domenic dismissed their stale review Sep 13, 2018

Mattias found problems I did not, oops

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Sep 13, 2018

Let's work on a test plan together to guide @devsnek. Here's a start:

  • 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
  • Cancelation 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)

Can anyone think of something else that might be missing?

Copy link
Contributor

MattiasBuelens left a comment

One more thing we missed in the spec text: next() and return() should return rejected promises instead of throwing. @domenic got it right in their proposed test plan.

index.bs Outdated
<h4 id="rs-asynciterator-prototype-next" method for="ReadableStreamAsyncIteratorPrototype">next()</h4>

<emu-alg>
1. If ! IsReadableStreamAsyncIterator(*this*) is *false*, throw a *TypeError* exception.

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Sep 14, 2018

Contributor

This should return a rejected promise, rather than throw an exception. From this comment by @domenic:

Calling next() or return() on non-ReadableStreamDefaultReaderAsyncIterator instances rejects with TypeError (there are existing brand check tests you can add to)

index.bs Outdated
for="ReadableStreamAsyncIteratorPrototype">return( <var>value</var> )</h4>

<emu-alg>
1. If ! IsReadableStreamAsyncIterator(*this*) is *false*, throw a *TypeError* exception.

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Sep 14, 2018

Contributor

Same as above, this should return a rejected promise.

Copy link
Contributor

MattiasBuelens left a comment

One tiny typo left. Other than that, spec text looks good. 👍

index.bs Outdated
<h4 id="rs-asynciterator-prototype-next" method for="ReadableStreamAsyncIteratorPrototype">next()</h4>

<emu-alg>
1. If ! IsReadableStreamAsyncIterator(*this*) is *false*, <a>a promise rejected with</a> *TypeError* exception.

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Sep 16, 2018

Contributor

The word "return" is missing in this sentence.

return Promise.reject(streamAsyncIteratorBrandCheckException('next'));
}
const reader = this._asyncIteratorReader;
if (this.preventCancel === false) {

This comment has been minimized.

Copy link
@ricea

ricea Sep 18, 2018

Collaborator

Should be this._preventCancel.

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Sep 18, 2018

Contributor

Good catch! This would have been an embarrassing bug. 😛

}
const reader = this._asyncIteratorReader;
if (reader._ownerReadableStream === undefined) {
return Promise.reject(readerLockException('next'));

This comment has been minimized.

Copy link
@ricea

ricea Sep 18, 2018

Collaborator

Maybe this should be readerLockException('iterate')?

"Cannot next a stream using a released reader" doesn't make much sense.

const reader = this._asyncIteratorReader;
if (this.preventCancel === false) {
if (reader._ownerReadableStream === undefined) {
return Promise.reject(readerLockException('next'));

This comment has been minimized.

Copy link
@ricea

ricea Sep 18, 2018

Collaborator

This should not be 'next'. Maybe 'finish iterating' makes sense?

index.bs Outdated
1. If ! IsReadableStreamAsyncIterator(*this*) is *false*, return <a>a promise rejected with</a> *TypeError* exception.
1. Let _reader_ be *this*.[[asyncIteratorReader]].
1. If *this*.[[preventCancel]] is *false*, then:
1. If _reader_.[[ownerReadableStream]] is *undefined*, return <a>a promise rejected with</a> a *TypeError*

This comment has been minimized.

Copy link
@ricea

ricea Sep 18, 2018

Collaborator

These two steps are in both branches of the if statement and so should be moved up above it.

This comment has been minimized.

Copy link
@MattiasBuelens

MattiasBuelens Sep 18, 2018

Contributor

This should also be changed in the reference implementation.

@ricea

This comment has been minimized.

Copy link
Collaborator

ricea commented Sep 18, 2018

Can anyone think of something else that might be missing?

We also need a test to verify that it inherits from %AsyncIteratorPrototype%. I don't know the best way to do that.

@ricea

This comment has been minimized.

Copy link
Collaborator

ricea commented Sep 19, 2018

Looks good. Are you working on some web platform tests?

@devsnek

This comment has been minimized.

Copy link
Author

devsnek commented Sep 20, 2018

@ricea yeah, its coming along. i'll have a pr up this weekend hopefully.

@ricea

This comment has been minimized.

Copy link
Collaborator

ricea commented Oct 4, 2018

@devsnek How is it going? Could you upload a work-in-progress PR of the tests so I can help out?

@devsnek

This comment has been minimized.

Copy link
Author

devsnek commented Oct 4, 2018

@ricea sorry its been taking me so long to finish this up. here's what i've done so far: web-platform-tests/wpt#13362

@ricea

This comment has been minimized.

Copy link
Collaborator

ricea commented Oct 4, 2018

@devsnek Thanks, I had a quick look and it looks good. I will pick through it in more detail tomorrow.

@ricea ricea mentioned this pull request Nov 6, 2018
@domenic domenic mentioned this pull request Nov 7, 2018
@wmhilton

This comment has been minimized.

Copy link

wmhilton commented Dec 29, 2018

I'm really excited about this! Looking forward to it landing. 🍻

@ricea

This comment has been minimized.

Copy link
Collaborator

ricea commented Jan 22, 2019

@devsnek I can take this over if you are busy. Please let me know.

@MattiasBuelens

This comment has been minimized.

Copy link
Contributor

MattiasBuelens commented Jan 22, 2019

Oh right, I previously said that I could also pick this up in my free time if needed. But if you want to take it over, that's also fine by me, then I'll help with the review instead. 🙂

@devsnek

This comment has been minimized.

Copy link
Author

devsnek commented Jan 22, 2019

@ricea @MattiasBuelens feel free to take over

@ricea

This comment has been minimized.

Copy link
Collaborator

ricea commented Jan 24, 2019

Oh right, I previously said that I could also pick this up in my free time if needed. But if you want to take it over, that's also fine by me, then I'll help with the review instead.

Okay, well since you offered first, you should have first opportunity to do it. If you're busy I'll pick it up in a week or so.

@MattiasBuelens

This comment has been minimized.

Copy link
Contributor

MattiasBuelens commented Jan 24, 2019

Okay, well since you offered first, you should have first opportunity to do it. If you're busy I'll pick it up in a week or so.

Thanks! 😄 I think I can look into it over the weekend.

Just to check: I should branch off from web-platform-tests/wpt#13362, and then submit a new PR, right? Since I obviously can't push commits to a branch I don't own. And if further changes are needed on the spec or reference implementation, those can also go into a new PR?

@ricea

This comment has been minimized.

Copy link
Collaborator

ricea commented Jan 24, 2019

Just to check: I should branch off from web-platform-tests/wpt#13362, and then submit a new PR, right? Since I obviously can't push commits to a branch I don't own. And if further changes are needed on the spec or reference implementation, those can also go into a new PR?

Sounds good to me.

@devsnek devsnek closed this Jan 26, 2019
@MattiasBuelens MattiasBuelens mentioned this pull request Jan 26, 2019
28 of 28 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.