WIP: Remove StreamSink #5744
Draft
WIP: Remove StreamSink #5744
Conversation
This file contains hidden or 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
The design of `StreamSink` makes it fairly complicated and somewhat inefficient to implement. For example: * It requires the use of `setPipeline()` so that the caller can start to enable promise pipelining on the `StreamSink` before it actually returns results. * Every call must create a `resultsStreamSink` object in advance, before it knows if there will be any streams in the results. * Generally there's just a lot of contortions involved in supporting it. It occurred to me that a different design is possible: one where we have an object that is created *per-IoContext* (instead of per-call) which can be used to "push" values into that IoContext, so that they can then be referenced as externals by subsequent JsValues. This commit introduces that design, including specifying how it would work to implement `ReadableStream`. Subsequent commits will actually implement this design. Eventually, after everyone in production is updated to understand and then use the new design, we can deprecate and remove StreamSink, thus cleaning up all the mess it created.
capnp PR: capnproto/capnproto#2475 The comment in this file says not to hand-edit it, but I don't understand how to use update-deps.py to update to a not-yet-merged PR...
This doesn't yet support any actual pushed types, this is just the infrastructure needed to make it available. This introduces an autogate which opts into using ExternalPusher for calls. The caller decides (based on the autogate) whether to use ExternalPusher for the whole call. We can turn on the autogate once all call receivers in production understand the new protocol.
Currently, it appears that we do not run wd-tests with all-autogates. We probably should but that'll have to be a separate change. For now, this arranges just to run js-rpc-test and abortsignal-test a second time with the ExternalPusher autogate on.
DO NOT MERGE until the autogate introduced in #5738 is fully rolled out and working. Until then I'm putting this up just to show what we'll be able to remove.
kentonv
force-pushed
the
kenton/rpc-external-pusher
branch
3 times, most recently
from
e420f35 to
513626f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
DO NOT MERGE until the autogate introduced in #5738 is fully rolled out and working.
Until then I'm putting this up just to show what we'll be able to remove.