Refactor: Intorduce new JsValue.ExternalPusher interface to replace StreamSink. #5738
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
kentonv
force-pushed
the
kenton/rpc-external-pusher
branch
from
15f0180 to
27b2baf
Compare
Member
Author
|
This PR sets up for some other changes I need to make, including:
|
CodSpeed Performance ReportMerging #5738 will not alter performanceComparing Summary
Footnotes
|
kentonv
force-pushed
the
kenton/rpc-external-pusher
branch
from
27b2baf to
e9605b0
Compare
Member
Author
|
Bolted awake at 6AM realizing: I broke e-order. Ugh. |
kentonv
added a commit
to capnproto/capnproto
that referenced
this pull request
Dec 22, 2025
This is a bit of a hack, but works in certain cases where we know for sure that promises are already resolved, including a case I'm working on in: cloudflare/workerd#5738
kentonv
force-pushed
the
kenton/rpc-external-pusher
branch
3 times, most recently
from
cbc6d8a to
c2af264
Compare
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.
kentonv
force-pushed
the
kenton/rpc-external-pusher
branch
from
c2af264 to
88b293e
Compare
|
The generated output of |
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...
kentonv
force-pushed
the
kenton/rpc-external-pusher
branch
from
88b293e to
fd2e29a
Compare
Member
Author
|
OK I fixed e-order and actually simplified this quite a bit in the process. It did require a hack in capnp: capnproto/capnproto#2475 But it works OK here. |
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.
kentonv
force-pushed
the
kenton/rpc-external-pusher
branch
from
fd2e29a to
e7fe4d0
Compare
kentonv
added a commit
that referenced
this pull request
Dec 22, 2025
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
added a commit
that referenced
this pull request
Dec 22, 2025
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.
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.
The design of
StreamSinkmakes it fairly complicated and somewhat inefficient to implement. For example:setPipeline()so that the caller can start to enable promise pipelining on theStreamSinkbefore it actually returns results.resultsStreamSinkobject in advance, before it knows if there will be any streams in the results.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 PR implements this design, guarded behind an autogate. The autogate should not be enabled anywhere in production until all of production has received the code update -- this ensures that all of prod can understand the new protocol before anyone tries to use it.
Once the autogate is fully rolled out, we can delete StreamSink, which should be a pleasing reduction in complexity -- more so than the complexity that this PR adds.
#5744 shows what we'll be able to delete once this is landed. While the total quantity of code is neutral, I feel that StreamSink was a lot more complicated in the implementation details.