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

ricea commented on this pull request.



> @@ -554,10 +554,14 @@ function ReadableStreamTee(stream, cloneForBranch2) {
   });
 
   function pullAlgorithm() {
-    return ReadableStreamDefaultReaderRead(reader).then(result => {
-      if (closed === true) {
-        return;
-      }
+    if (pulling === true) {

You could use `ReadableStreamGetNumReadRequests(stream) > 0` instead of having a separate `pulling` variable to keep track of. However, using a `pulling` variable makes it more obviously correct, so on balance I prefer it.

Nit: I think `reading` is a better name than `pulling`, since it really reflects the duration of the call to ReadableStreamDefaultReaderRead.

> @@ -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);

Good point, I missed this in my implementation.

> @@ -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;

I like the idea of always returning Promise.resolve() as it makes the behaviour more symmetrical.

I don't think we can wrap the whole function in another if(), as it will increase the number of levels of indentation, which is bad for the readability of the standard text. We still have to do an early exit.

Of course, we still need abundant tests of error conditions to be confident it works.

-- 
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-207414927