Settle reader.[[closedPromise]] before performing close/error steps of read requests

[MattiasBuelens]

At least two implementations (#1100, MattiasBuelens/web-streams-polyfill#66) have made the (incorrect) assumption that reader.[[closedPromise]] will only be resolved or rejected if the promise is still pending. This is currently not the case: an async iterator may release its lock (and reject reader.[[closedPromise]]) right before the stream tries to resolve that same promise again.

With this PR, we now resolve or reject reader.[[closedPromise]] before going through all pending read requests and running their close or errors steps. This ensures that the stream is already done with all of its own state updates, and the read request's steps can observe and manipulate the "final" state of the stream. Thus, when the async iterator releases its lock, it will replace the [[closedPromise]] with a newly rejected promise instead.

This is a small but observable change for both web authors and other specifications:

• For web authors, reader.closed will resolve with undefined before reader.read() resolves with { done: true, value: undefined }.
• For other specifications using the "read a chunk" algorithm, when their close steps are run, they will now observe that reader.closed is already resolved (see comment on #1100).

There was also a small bug in the reference implementation of the WebIDL promise helpers. Calling resolvePromise or rejectPromise on a promise created with promiseResolvedWith or promiseRejectedWith would throw an error (such as "promiseSideTable.get(p).resolve is not a function"). If we attempt to resolve or reject a promise that is already resolved or rejected, we should do nothing instead.

• At least two implementers are interested (and none opposed) (skipped):
  • Chrome (@ricea)
• Tests are written and can be reviewed and commented upon at:
  • Streams: settle reader.[[closedPromise]] before performing close/error steps of read requests web-platform-tests/wpt#27236
• Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1175965
  • Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1691559
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=221576

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[domenic]

LGTM. As shown in https://github.com/web-platform-tests/wpt/pull/27236/files this is a change for all implementations though, so we'll want bugs filed at the very least.

@ricea can you check this out and see if the change makes sense to you, and is worth doing in Chromium? If you as an implementer think that maybe we shouldn't bother with the churn, then that's also a worthwhile sign...

[ricea]

Although this change is not observable for web authors (since all operations on a reader or async iterator have at least one microtask delay), it may be observable for other specifications (see comment on #1100).

As you've found, it is observable, so please remove this sentence. 😄

There was also a small bug in the reference implementation of the WebIDL promise helpers. Calling resolvePromise or rejectPromise on a promise created with promiseResolvedWith or promiseRejectedWith would throw an error (such as "promiseSideTable.get(p).resolve is not a function"). If we attempt to resolve or reject a promise that is already resolved or rejected, we should do nothing instead.

I believe the intent was to try to ensure that a promise in the standard would never be resolved or rejected more than once. In practice, we've never managed to completely eliminate such errors, so all implementations have to guard against them anyway.

On balance, I think it's still worth trying to avoid multiple resolves / rejects in the reference implementation and standard, since it helps us catch some kinds of bugs.

[ricea]

I liked the old behaviour of resolving read() before closed, but you've shown it's error-prone. So I think this change is worthwhile.

[MattiasBuelens]

As you've found, it is observable, so please remove this sentence. 😄

Good point. Updated the PR description. 🙂

On balance, I think it's still worth trying to avoid multiple resolves / rejects in the reference implementation and standard, since it helps us catch some kinds of bugs.

Would it make sense for the promise helpers in the reference implementation to assert that the promise is still pending? That is, change these lines in resolvePromise and rejectPromise:

- if (promiseSideTable.get(p).stateIsPending === false) {
-   return;
- }
+ assert(promiseSideTable.get(p).stateIsPending === true);

We could do this only in the reference implementation, just to help catch bugs. Or would we also want to add normative asserts everywhere we resolve or reject a promise in the specification text?

[ricea]

Would it make sense for the promise helpers in the reference implementation to assert that the promise is still pending?

Yes, that would be good. Could you do it in this PR?

We could do this only in the reference implementation, just to help catch bugs. Or would we also want to add normative asserts everywhere we resolve or reject a promise in the specification text?

I think we shouldn't make it normative in the specification text, because we've so far always found cases where we failed to preserve the invariant. @domenic may have a different opinion.

[domenic]

I agree with reference implementation asserts (probably with a comment) and no spec changes.

I kind of thought in this scenario the double-resolve was unavoidable, so the assert would fail? But I guess I missed something.

[MattiasBuelens]

I kind of thought in this scenario the double-resolve was unavoidable, so the assert would fail? But I guess I missed something.

Nope.

At the start of ReadableStreamClose, the stream is still "readable". So if it is locked to a reader, that reader's [[closedPromise]] is still pending. Originally, we would first change the state to "closed", then run the close steps of all read requests, and only then resolve [[closedPromise]]. We assumed that [[closedPromise]] cannot be changed by those close steps, and thus is still pending when we try to resolve it. This was incorrect: if one of those close steps calls ReadableStreamGenericRelease, [[closedPromise]] will have been replaced with a rejected promise, and we'd be resolving an already rejected promise.

We now swap the order around: first change the state to "closed", resolve [[closedPromise]], and then run the close steps. The close steps are still the same: ReadableStreamGenericRelease will still replace the (now resolved instead of pending) [[closedPromise]] with a rejected promise. But we don't need to do anything else afterwards in ReadableStreamClose, so we no longer need to worry about what those close steps might have changed. 😉

[MattiasBuelens]

There is an alternative solution which retains the old behavior, i.e. resolving [[closedPromise]] after calling the close steps:

1. After running the close steps, ReadableStreamClose must re-acquire stream.[[reader]], since the close steps could have released the lock of the current reader, and possibly even acquired a new reader.
2. ReadableStreamClose must first check if reader.[[closedPromise]] is still pending, since the close steps may have released the lock and changed it to a rejected promise. If it is not pending, it must do nothing, i.e. it must not replace the current promise with a resolved one.
3. In step 3 of ReadableStreamReaderGenericRelease, we cannot use reader.[[stream]].[[state]] to determine whether we can reject [[closedPromise]] or we need to replace it. As discussed earlier, when this is run as part of the close steps of a read request, the stream's state will already be "closed" but our [[closedPromise]] is still pending. Thus, we would need to change this step to check directly whether [[closedPromise]] is still pending. (This does align a bit closer with WritableStreamDefaultWriterEnsureReadyPromiseRejected though.)

But you'd still have the (kind of weird) situation where stream.[[state]] and reader.[[closedPromise]] don't agree inside the close steps of a read request... 😕

I think this alternative solution is more difficult to understand and maintain than the current solution. I still prefer the current one. 😁

[ricea]

I think this alternative solution is more difficult to understand and maintain than the current solution. I still prefer the current one. 😁

Yes, I prefer the current one too. The alternative solution is too complex for me to have confidence that it is right.

[domenic]

Alright, let's land this. I'll merge the WPT PR; @MattiasBuelens would you be able to roll the tests one last time and then file browser bugs?

As editor, I'm willing to skip gathering affirmative implementer interest for this sort of minor barely-observable internal bugfix, on the reasoning that we can revisit if there's any pushback (which seems highly unlikely). As long as we have one implementer (viz. @ricea) saying this is worthwhile.

[ricea]

lgtm from me too.

[MattiasBuelens]

@MattiasBuelens would you be able to roll the tests one last time and then file browser bugs?

Sure thing! I'll roll WPT after the tests PR has landed. 🙂

[MattiasBuelens]

Implementation bugs are filed, links are in the PR description. Ready to merge.