add @@asyncIterator to ReadableStream by devsnek · Pull Request #954 · whatwg/streams

devsnek · 2018-09-07T18:34:14Z

Closes #778

domenic · 2018-09-07T18:55:08Z

index.bs

+
+  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 copy-pasted text seems not as useful as the old text.

domenic · 2018-09-07T18:55:26Z

index.bs

 </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>


What happened to getIterator()?

domenic · 2018-09-07T18:55:49Z

index.bs

+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}}


Typo "implement" (maybe my fault).

index.bs

domenic · 2018-09-07T18:58:51Z

index.bs

+    exception.
+    1. Let _result_ be ! ReadableStreamReaderGenericCancel(_reader_, _value_).
+  1. Otherwise,
+    1. Let _result_ be ! ReadableStreamCreateReadResult(_value_, *true*, *true*).


This needs to be a promise resolved with...

index.bs

domenic · 2018-09-07T19:17:10Z

index.bs

+  1. Otherwise,
+    1. Let _result_ be _value_.
+  1. Perform ! ReadableStreamReaderGenericRelease(_reader_).
+  1. Return <a>a promise resolved with</a> ! ReadableStreamCreateReadResult(_result_, *true*, *true*).


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.

domenic

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

MattiasBuelens

Still some holes left in return().

MattiasBuelens · 2018-09-07T23:32:43Z

index.bs

+    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 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.

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.

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!

MattiasBuelens · 2018-09-07T23:36:37Z

index.bs

+    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_).


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.

Mattias found problems I did not, oops

domenic · 2018-09-13T18:30:40Z

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 (add @@asyncIterator to ReadableStream #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 (add @@asyncIterator to ReadableStreamDefaultReader #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?

MattiasBuelens

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.

MattiasBuelens · 2018-09-14T08:43:51Z

index.bs

+<h4 id="rs-asynciterator-prototype-next" method for="ReadableStreamAsyncIteratorPrototype">next()</h4>
+
+<emu-alg>
+  1. If ! IsReadableStreamAsyncIterator(*this*) is *false*, throw a *TypeError* exception.


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)

MattiasBuelens · 2018-09-14T08:44:12Z

index.bs

+for="ReadableStreamAsyncIteratorPrototype">return( <var>value</var> )</h4>
+
+<emu-alg>
+  1. If ! IsReadableStreamAsyncIterator(*this*) is *false*, throw a *TypeError* exception.


Same as above, this should return a rejected promise.

MattiasBuelens

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

MattiasBuelens · 2018-09-16T14:45:22Z

index.bs

+<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.


The word "return" is missing in this sentence.

ricea · 2018-09-18T04:40:10Z

reference-implementation/lib/readable-stream.js

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


Should be this._preventCancel.

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

ricea · 2018-09-18T04:44:02Z

reference-implementation/lib/readable-stream.js

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


Maybe this should be readerLockException('iterate')?

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

ricea · 2018-09-18T04:44:44Z

reference-implementation/lib/readable-stream.js

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


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

ricea · 2018-09-18T04:48:03Z

index.bs

+  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*


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

This should also be changed in the reference implementation.

ricea · 2018-09-18T04:51:46Z

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 · 2018-09-19T23:52:36Z

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

devsnek · 2018-09-20T00:07:43Z

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

ricea · 2018-10-04T13:52:34Z

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

devsnek · 2018-10-04T14:25:19Z

@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 · 2018-10-04T14:34:43Z

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

billiegoose · 2018-12-29T16:03:39Z

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

ricea · 2019-01-22T10:07:22Z

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

MattiasBuelens · 2019-01-22T12:36:50Z

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 · 2019-01-22T17:31:51Z

@ricea @MattiasBuelens feel free to take over

ricea · 2019-01-24T08:33:00Z

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 · 2019-01-24T09:55:26Z

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 · 2019-01-24T10:00:45Z

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.

add @@asynciterator to ReadableStream

6c96626

domenic reviewed Sep 7, 2018

View reviewed changes

devsnek added 2 commits September 7, 2018 14:11

squash! nits

7e7aaa1

squash! add cross-links

b682fdc

domenic reviewed Sep 7, 2018

View reviewed changes

squash! transform cancel promise

d08405f

domenic previously approved these changes Sep 7, 2018

View reviewed changes

MattiasBuelens requested changes Sep 7, 2018

View reviewed changes

MattiasBuelens requested changes Sep 14, 2018

View reviewed changes

squash! stuff

d84893a

MattiasBuelens requested changes Sep 16, 2018

View reviewed changes

squash! more

8f37db0

ricea reviewed Sep 18, 2018

View reviewed changes

squash! more

291da25

devsnek mentioned this pull request Oct 4, 2018

[WIP] ReadableStream @@asyncIterator tests web-platform-tests/wpt#13362

Closed

ricea mentioned this pull request Nov 6, 2018

Let's do this nodejs/whatwg-stream#1

Open

domenic mentioned this pull request Nov 7, 2018

Switching to Web IDL #963

Closed

MattiasBuelens mentioned this pull request Jan 26, 2019

Add @@asyncIterator to ReadableStream #980

Merged

devsnek closed this Jan 26, 2019

MattiasBuelens mentioned this pull request Jan 26, 2019

ReadableStream @@asyncIterator web-platform-tests/wpt#15097

Merged

28 tasks

devsnek deleted the feature/rs-asynciterator branch July 8, 2021 18:47

Comments

Conversation

devsnek commented Sep 7, 2018 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

MattiasBuelens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic commented Sep 13, 2018

Uh oh!

MattiasBuelens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattiasBuelens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricea commented Sep 18, 2018

Uh oh!

ricea commented Sep 19, 2018

Uh oh!

devsnek commented Sep 20, 2018

Uh oh!

ricea commented Oct 4, 2018

Uh oh!

devsnek commented Oct 4, 2018

Uh oh!

ricea commented Oct 4, 2018

Uh oh!

billiegoose commented Dec 29, 2018

Uh oh!

ricea commented Jan 22, 2019

Uh oh!

MattiasBuelens commented Jan 22, 2019

Uh oh!

devsnek commented Jan 22, 2019

Uh oh!

ricea commented Jan 24, 2019

Uh oh!

MattiasBuelens commented Jan 24, 2019

devsnek commented Sep 7, 2018 •

edited by pr-preview bot

Loading