Transform backpressure approach 2 request for comments #110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this commit, BaseReadableStream would immediately exert backpressure (i.e. return `false` from `push`), even if the buffer was empty at the time of being pushed into. After it, backpressure will only be exerted if the buffer is nonempty. This sets the stage for (I think) solving #24, since before this commit, it was impossible for a BaseReadableStream to ever return `true` from `push`. After it, that is possible, as long as the stream clears its internal buffer as fast as it can.
Similarly to the previous commit for readable streams, you can push one chunk into the buffer of a writable stream before it transitions to the waiting state. That way, if the chunk gets communicated to the underlying source immediately, the stream stays writable forever, and never communicates backpressure backward through the chain unnecessarily.
When piping: - The stream enters a "waiting" state until it closes. It is never directly readable. - Calling read() gives a TypeError. (An informative one!) - Calls to `push` forward themselves directly into the destination's `write` call, within a single tick. This is solution 2 to the problem mentioned in #24 (comment).
…nly) This reestablishes the meaning of the [[pulling]] flag to mean "is the `pull` callback executing right now," so that it can be used to avoid infinitely-recursive pull calls. The meaning of [[pulling]] has gotten a bit confused. It does two things right now: prevent "too many" calls to `pull` in a row, and avoid reentrant calls in the pipe use case. (Reentrant calls are not possible in the non-pipe use case, since those will necessarily be mediated through wait(). I think.) Notably you can remove the line that checks [[pulling]] inside [[callPull]] without any tests failing except the one that is explicitly checking that `pull` is not called multiple times.
|
I've merged in the first two commits. The direct-piping, I will hold off on for now, as I think it might be premature optimization, and I agree that the stack growth is a concern, as well as other problems regarding non-clean-stack execution of user code (similar to the problems with promises). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commit messages for details. /cc @othiym23 @tyoshino
The first two commits seem like a good idea to me, independent of their surroundings. Especially the writable one. The readable one is a bit sketchier, since without the next two commits, there's no way for consumers to know that they should be picking up data ASAP.
The next two commits are using the reference implementation + tests to prototype approach 2 from #24 (comment). Take a look at the tests especially, since those highlight what I'm hoping to accomplish.
The additional internal complexity introduced is sad. But it has some benefits, I think:
read()from a stream that is being piped elsewhere. The API is explicitly designed around single-consumer, with problems occurring if you try to violate that. This revision makes that explicit, by erroring if you try toread()from a piped stream.That said, I still want to explore the solution space here heavily. I'll probably do a branch with approach 5 as well, and I had an idea for approach 4 that I will reiterate back over in that thread.