ReadableStreamClose with async iterator and pending read requests errors

[kitsonk]

In implementing the Streams spec in Deno, I came across a regression that I think might be a spec error. We had a test which attempted to close an async iterable on a ReadableStream before all the reads had completed (reading a body returned from fetch).

According to the spec for ReadableStreamClose, for a ReadableStreamDefaultReader any pending read requests close steps should be performed (5.1.1) before resolving the reader.[[closedPromise]].

The read request from an iterators next steps set the closed steps to call ReadableStreamReaderGenericRelease, which will reject the closed promise (either step 3 or 4).

That means when we get to step 6 of ReadableStreamClose the promise is already fulfilled, which in the case of our implementation threw an error. I have added a guard step to determine if the [[closedPromised]] is already fulfilled at step 6, but I don't know if that is the best long term solution, or if I have some other mis-understanding about the spec, where it should be impossible for the [[closedPromise]] to already be settled that that step.

[domenic]

I haven't done an analysis of this particular case, so there might indeed be a logic error in terms of what's intended. But I'll note that generally in promises, including in specs, resolving a promise that's already settled (e.g. already rejected) is a no-op:

let resolve;
const p = new Promise((r, reject) => {
  reject(new Error());
  resolve = r;
});

resolve(); // does nothing

Again, maybe the extra resolve attempt indicates we're doing something wrong; we'll need a bit more time to do the analysis to figure out what the best behavior is. But at least one interpretation is that it should not cause an error, and just no-op. (If that's the case, then the action item is probably to add a note to the spec explaining that the resolve could be a no-op in that step.)

[kitsonk]

Thanks, that makes sense. Our implementation of "unwrapped" promises was preventing double fulfilment, out of an abundance of caution, but as you state, that should just be a no-op. So instead of guarding, I will change the implementation of our "unwrapped" promises.

[MattiasBuelens]

I encountered a similar issue in MattiasBuelens/web-streams-polyfill#66. I solved it in the same way: ensure that resolving/rejecting an already settled promise is a no-op (and does not throw). It's a bit surprising, but I understand the reasoning.

I also added a regression test for this case. If you're interested, I can look into upstreaming it to WPT? 🙂

[domenic]

@MattiasBuelens, have you done the analysis of this case such that you're convinced the current behavior is correct? If so, upstreaming to WPT and adding a note to the spec about this possibility sounds nice.

[kitsonk]

@MattiasBuelens that regression test case looks almost exactly like the test case that triggered the error in our implementation. I would be 👍 for upstreaming it.

[MattiasBuelens]

@domenic I did have a look. I see one possible improvement, but it's debatable.

We could move the "resolve/reject reader.[[closedPromise]]" steps in ReadableStreamClose and ReadableStreamError before the loops that call the close/error steps of each read request. This makes the behavior a bit more predictable: the closed promise always becomes resolved or rejected first, regardless of what the closed/error steps do.

Although we would still reject both the read() and closed promises in ReadableStreamClose, the relative order of these rejections would change. This would affect at least one WPT test, since it asserts that the rejection handler for read() is called before the rejection handler for closed. I'm not sure how important this really is...? 🤷

[domenic]

I tried digging in to this a bit, but I'm still not sure I've got this all loaded into my head correctly... please let me know if I'm off base on any of the following.

Although we would still reject both the read() and closed promises in ReadableStreamClose, the relative order of these rejections would change.

From what I can tell, ReadableStreamClose resolves the reader's [[closedPromise]] with undefined, and sets the [[state]] to "closed". So then the loop would repace [[closedPromise]] with a new rejected promise, I think. Is that what you mean?

More generally, I'm wondering if this problem is reproducible outside the framework of async iterators. Isn't something like the following very similar?

reader.read(({ value, done }) => {
  if (done) {
    reader.releaseLock();
  }
});

I guess that one is saved by the extra microtask delay, which effectively makes the release call happen after the state has already become "closed"?

If that's the case, then maybe rearranging the steps would be better, so that the async iterator close steps behave similarly to such a code snippet...

And also just from a general code hygeine perspective, ReadableStreamClose does look a little weird: it should probably keep [[state]] setting closer to [[closedPromise]] setting, without the arbitrary spec code of the close steps running in between.

[MattiasBuelens]

From what I can tell, ReadableStreamClose resolves the reader's [[closedPromise]] with undefined, and sets the [[state]] to "closed". So then the loop would repace [[closedPromise]] with a new rejected promise, I think. Is that what you mean?

Yup, that's correct.

More generally, I'm wondering if this problem is reproducible outside the framework of async iterators. Isn't something like the following very similar?

I guess that one is saved by the extra microtask delay, which effectively makes the release call happen after the state has already become "closed"?

Indeed, it's not reproducible with reader.read() because of the microtask delay.

Async iterators are a bit special, in that they can synchronously release the reader's lock in response to a read request. (But then again, this isn't really observable, since async iterators don't actually use [[closedPromise]].)

It could be observable in other specifications though. When using the "read a chunk" algorithm, the close steps also run synchronously. So they could observe that the stream is closed while reader.closed is still pending. It's very unlikely any specification would actually do this, but it's theoretically possible. 😛

If that's the case, then maybe rearranging the steps would be better, so that the async iterator close steps behave similarly to such a code snippet...

And also just from a general code hygeine perspective, ReadableStreamClose does look a little weird: it should probably keep [[state]] setting closer to [[closedPromise]] setting, without the arbitrary spec code of the close steps running in between.

Agreed. I think it's good practice to perform all the necessary updates to our own state first, and then perform any registered "callbacks" (close or error steps) at the end. I feel it makes things easier to reason about. 🙂

---

If you're okay with it, I can make a PR to move those steps around in the specification and update that single WPT test to expect a slightly different order. (Or alternatively: maybe we should make the test not check the order of reader.read() and reader.closed becoming rejected? It doesn't seem that important. 🤷‍♂️)

Oh, right, I'll also include my regression test case then. 😄

[domenic]

If you're okay with it, I can make a PR to move those steps around in the specification and update that single WPT test to expect a slightly different order

That would be lovely!

Or alternatively: maybe we should make the test not check the order of reader.read() and reader.closed becoming rejected? It doesn't seem that important

I agree it's not that important, but I think more coverage is nicer for anything observable. We can always change things around as we go.

[MattiasBuelens]

Interesting! The reference implementation fails my new test. 😛

In the current version, we first call the close/error steps of all read requests, and afterwards we resolve/reject the reader's closed promise. When the close/error steps of a read request release the reader's lock, the stream's state is already 'closed' so we use promiseRejectedWith to replace the closed promise (which is actually still pending!) with a newly rejected promise. However, when we later on try to resolve the closed promise in ReadableStreamClose, it throws on this line because there is no resolve method in the entry of our promise side table! 😱 This happens because promiseRejectedWith creates an entry with only the stateIsPending property, no resolve or reject properties.

In short: resolvePromise(promiseRejectedWith(reason), value) throws in the reference implementation. This is just a silly bug in our reference implementation though, browsers that properly implement the WebIDL promise helpers shouldn't have this problem. Still, I'll fix our reference implementation to return immediately if stateIsPending is already false, before trying to call resolve or reject. 😉