Re: [whatwg/streams] Add @@asyncIterator to ReadableStream (#980)

The tests have found a bug in the spec! 🎉 

[This test](https://github.com/web-platform-tests/wpt/blob/ec6765af7c0cbd53bd8c3e9cfab1e54d5fee8e6a/streams/readable-streams/async-iterator.any.js#L216) fails with the current reference implementation. When calling `s.getReader()`, the stream is still locked to the iterator's reader.

The problem is that `it.return()` is not called whenever a `for await..of` loop **completes normally** (i.e. when `it.next()` resolves with `{done: true}`). A quick snippet to demonstrate:
```js
function testIterator() {
  return {
    [Symbol.asyncIterator]() {
      return {
        async next() {
          console.log('next() called');
          return {done: true};
        },
        async return() {
          console.log('return() called');
        }
      };
    }
  };
}

(async () => {
  console.log('before');
  for await (const chunk of testIterator()) {
    console.log({chunk});
  }
  console.log('after');
})();
```
This logs:
```
before
next() called
after
```
Clearly, the `return()` method was not called.

To fix this, we need to change the `next()` method to release the reader just before returning `{done: true}`. For example, we could change [this line in the reference implementation](https://github.com/whatwg/streams/blob/173af93b61a91032c1d270f2215c0683c95d3857/reference-implementation/lib/readable-stream.js#L184) to the following:
```js
    return ReadableStreamDefaultReaderRead(reader)
      .then(({ value, done }) => {
        if (done) {
          ReadableStreamReaderGenericRelease(reader);
        }
        return ReadableStreamCreateReadResult(value, done);
      });
```
Similarly, the spec text should be updated to transform the returned promise with this fulfillment handler.

@domenic @ricea @devsnek Thoughts?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/980#issuecomment-457932482