Re: [whatwg/streams] ReadableStreamTee: do not read when already pulling (#997)

MattiasBuelens commented on this pull request.



> @@ -592,6 +595,13 @@ function ReadableStreamTee(stream, cloneForBranch2) {
         ReadableStreamDefaultControllerEnqueue(branch2._readableStreamController, value2);
       }
     });
+
+    // If pullPromise rejects with an AssertionError, but the branch is already closed or errored,
+    // the rejection will be silently ignored by ReadableStreamDefaultControllerError.
+    // We have to manually rethrow any AssertionError to make sure they are not ignored.
+    pullPromise.catch(rethrowAssertionErrorRejection);

Concretely: if the implementation has a bug that causes an assert failure in `ReadableStreamDefaultControllerClose` or `ReadableStreamDefaultControllerEnqueue`, this failure would be ignored if the branch was previously cancelled. This line ensures that an assert failure *always* causes an unhandled exception.

> @@ -592,6 +595,13 @@ function ReadableStreamTee(stream, cloneForBranch2) {
         ReadableStreamDefaultControllerEnqueue(branch2._readableStreamController, value2);
       }
     });
+
+    // If pullPromise rejects with an AssertionError, but the branch is already closed or errored,
+    // the rejection will be silently ignored by ReadableStreamDefaultControllerError.
+    // We have to manually rethrow any AssertionError to make sure they are not ignored.
+    pullPromise.catch(rethrowAssertionErrorRejection);
+
+    return pullPromise;

Technically speaking, we could also just return `Promise.resolve()` here. We already have a `catch` handler on `reader._closedPromise` to error the branches when the original stream errors, so we don't really *need* to return any rejections from `pull()`. Then it would look more like:

```js
function pullAlgorithm() {
  if (pulling === false) {
    pulling = true;
    ReadableStreamDefaultReaderRead(reader).then(result => {
      pulling = false;
      // ...
    }).catch(rethrowAssertionErrorRejection);
  }
  return Promise.resolve();
}
```
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/997#pullrequestreview-207129306