Re: [whatwg/streams] Support teeing readable byte streams (#1114)

@MattiasBuelens commented on this pull request.

I've added handling for errors from cloning chunks.

Still gotta find some time to write tests for all these edge cases. 😛 

> +       1. Perform ! [$ReadableStreamDefaultControllerError$](|branch1|.[=ReadableStream/[[controller]]=], |cloneResult|.\[[Value]]).
+       1. Perform ! [$ReadableStreamDefaultControllerError$](|branch2|.[=ReadableStream/[[controller]]=], |cloneResult|.\[[Value]]).
+       1. [=Resolve=] |cancelPromise| with ! [$ReadableStreamCancel$](|stream|, |cloneResult|.\[[Value]]).

There's another re-entrancy risk here. `ReadableStreamDefaultControllerError` will call `readRequest.errorSteps(e)`, which *could* call both `branch1.cancel()` and `branch2.cancel()`. Thus, our `cancel1Algorithm` (or `cancel2Algorithm` depending on which branch is cancelled last) will call `ReadableStreamCancel(stream, e)` twice. This is "okay" since it'll immediately return an already-resolved promise, but we'll also try to *resolve* our `cancelPromise` twice, which we want to avoid.

Possible solutions:
* Swap the order: cancel first, error branches second. However, this doesn't really solve the problem, since the stream's cancel steps might *also* cancel the branches and make us cancel the stream *again*. I guess we could add a separate `canceled` flag to remember if we already called `ReadableStreamCancel` before, but that seems messy... 😕 
* Add an extra _"if `canceled1` is false or `canceled2` is false"_ check before cancelling.

I think I'll go with the second option, with a note about why this check is necessary. Not sure if we can trigger this edge case from WPT, because we always have at least one micro-task delay from the user-facing `reader.read()` promises.

>     ::
     1. [=Queue a microtask=] to perform the following steps:
      1. Set |reading| to false.
-     1. Let |value1| and |value2| be |value|.
-     1. If |canceled2| is false and |cloneForBranch2| is true, set |value2| to ?
-        [$StructuredDeserialize$](? [$StructuredSerialize$](|value2|), [=the current Realm=]).
-     1. If |canceled1| is false, perform ?
+     1. Let |chunk1| and |chunk2| be |chunk|.
+     1. If |canceled2| is false and |cloneForBranch2| is true,
+      1. Let |cloneResult| be [$StructuredClone$](|chunk2|).
+      1. If |cloneResult| is an abrupt completion,
+       1. Perform ! [$ReadableStreamDefaultControllerError$](|branch1|.[=ReadableStream/[[controller]]=], |cloneResult|.\[[Value]]).

Should we use the original error as-is (i.e. a `"DataCloneError"` `DOMException`), or should we do "let `cloneError` be a new `TypeError`" instead? 🤔

-- 
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/1114#pullrequestreview-682449601