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 tests for ReadableStream.from() #27009

Merged
merged 20 commits into from
Jun 7, 2023

Conversation

MattiasBuelens
Copy link
Contributor

Add tests for the new static method ReadableStream.from().

See whatwg/streams#1083 for the accompanying spec change.

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

Let's have tests for three horrible re-entrant cases:

  1. reader.read() is called inside next().
  2. reader.cancel() is called inside next().
  3. reader.cancel() is called inside return().

@MattiasBuelens
Copy link
Contributor Author

@ricea Great ideas! I've added tests for reader.cancel() inside both next() and return().

What would you want to see from a "reader.read() is called inside next()" test?

If you mean await reader.read(), then the stream will stall forever. The first read is waiting for the second read to resolve, but the second read is waiting for the pull promise from the first read to resolve before it can pull again.

let reader;
const iterable = {
  async next() {
    return await reader.read();
  },
  [Symbol.asyncIterator]: () => iterable
};
const rs = ReadableStream.from(iterable);
reader = rs.getReader();
await reader.read(); // stalls forever

If you mean calling reader.read() without awaiting it, then you'll keep creating next() and read() promises forever, and blow up the microtask queue.

let reader;
const iterable = {
  async next() {
    reader.read(); // calls next() again, which calls read() again, repeating forever
    return { value: 'a', done: false };
  },
  [Symbol.asyncIterator]: () => iterable
};
const rs = ReadableStream.from(iterable);
reader = rs.getReader();
await reader.read(); // blows up the microtask queue

Or do we only want to call read() inside next() only once?

let nextCalls = 0;
let reader;
const iterable = {
  async next() {
    nextCalls += 1;
    if (nextCalls === 1) {
      reader.read();
    }
    return { value: 'a', done: false };
  },
  [Symbol.asyncIterator]: () => iterable
};
const rs = ReadableStream.from(iterable);
reader = rs.getReader();
await reader.read(); // calls next() twice
await reader.read(); // calls next() once

@ricea
Copy link
Contributor

ricea commented Mar 23, 2021

New tests lgtm.

What would you want to see from a "reader.read() is called inside next()" test?

Or do we only want to call read() inside next() only once?

Yes. Calling it once should be sufficient to find any reentrancy bugs.

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

@youennf
Copy link
Contributor

youennf commented Jun 7, 2023

@MattiasBuelens, can we merge the WPT tests?

@MattiasBuelens
Copy link
Contributor Author

@youennf Yep, this is ready to go!

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.

4 participants