Skip to content

SQC-871 Implements socket serialization for transferring socket over RPC - #6863

Closed
Ltadrian wants to merge 3 commits into
cloudflare:mainfrom
Ltadrian:dlapid/jsrpc_socket
Closed

SQC-871 Implements socket serialization for transferring socket over RPC#6863
Ltadrian wants to merge 3 commits into
cloudflare:mainfrom
Ltadrian:dlapid/jsrpc_socket

Conversation

@Ltadrian

@Ltadrian Ltadrian commented Jul 6, 2026

Copy link
Copy Markdown
  • Adds TransferredSocketStream class that handles implementing underlying AsyncIoStream IO operations for deserializing socket
  • Socket deserialization unwraps raw kj streams and builds non-deferred stream to pass into TransferredSocketStream
  • Adds js-rpc-socket-test tests
  • Add outbound interceptor test to showcase full end to end use case of loopback binding

RPC
- Adds TransferredSocketStream class that handles implementing
  underlying AsyncIoStream IO operations
- Socket serialization/deserialzation unwraps raw kj streams and builds
  non-deferred sockets
- Fix and assert existing test that returns socket over RPC and can read
  bytes
- Add outbound interceptor test to showcase returning socket over RPC
@Ltadrian Ltadrian changed the title Implements socket serialization for transferring socket over RPC SQC-871 Implements socket serialization for transferring socket over RPC Jul 6, 2026
// Serialize the readable and writable streams as separate externals
readable->serialize(js, serializer);

writable->serialize(js, serializer);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware the the JsReadableStream and JsWritableStream work necessarily will impact this. JsReadableStream has already landed. readable is no longer a jsg::Ref<ReadableStream> but is wrapped by a JsReadableStream. The serialization story is still a work in progress.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you I wasn't aware of JsReadableStream before working on this. I've caught up on the implementation in the internal repo workerd and it looks like it hopefully be too much of a lift to refactor some of these parts. I'll keep my eye out for when this lands in public repo 😊 Do you think it would be a blocker to support both JsReadableStream/JsWritableStream before landing this socket serialization?

Comment thread src/workerd/api/sockets.c++
Comment thread src/workerd/api/sockets.c++
Comment thread src/workerd/api/streams/readable.h Outdated
@jasnell

jasnell commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

A few questions...

  1. How does socket upgrade (startTLS) work with a serialized socket?
  2. It looks like isDefaultFetchPort is not set?
  3. There doesn't seem to be any handling of opened state. A socket uses the openedPromise to signal when the socket is actually opened/established. From the looks of it here, it would be possible to serialize a still not-yet-open socket but the receiving side would appear to be opened? It's not clear.
  4. What if the socket is already being closed? Or is already closed?

@dom96 dom96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, like James mentioned, I'd repeat that currently as things stand there is a need for more synchronisation with the various socket states (closed/open). But perhaps that's planned since this is still a draft :)

In general though I think this is a good start. I left a few comments about various things I'd improve as well, but nothing major.

Comment thread src/workerd/api/tests/js-rpc-test.js
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++
Comment thread src/workerd/api/sockets.c++
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/io/worker-interface.capnp
@Ltadrian

Ltadrian commented Jul 9, 2026

Copy link
Copy Markdown
Author

A few questions...

  1. How does socket upgrade (startTLS) work with a serialized socket?
  2. It looks like isDefaultFetchPort is not set?
  3. There doesn't seem to be any handling of opened state. A socket uses the openedPromise to signal when the socket is actually opened/established. From the looks of it here, it would be possible to serialize a still not-yet-open socket but the receiving side would appear to be opened? It's not clear.
  4. What if the socket is already being closed? Or is already closed?

@jasnell

  1. In the case of Hyperdrive needs, socket upgrade isn't needed as Hyperdrive will handle TLS to the origin. In the generalized socket case I am not sure. I imagine that we would need to somehow allow the transferred socket to upgrade tls and start tls handshake but I would probably prefer if the socket has already been upgraded before transfer instead?
  2. Updated to add isDefaultFetchPort to worker-interface.capnp and serialize/deserialize ✅
  3. I thought about this for a while and I think throwing an error is fair if the socket is still pending/closed. I can't think of a good case where receiving a socket and needing to await on socket.opened would be a fair user experience. I would prefer that the socket is assumed to be opened and connected. I've gone ahead and implemented some checks on serialize to prevent serializing the socket when that state is pending/closed but fully open to other implementation ideas here
  4. Throwing error on serialize feels fair to me. There is still the case of where on serialize the socket is opened, but during transit the connection closes and we assume the serialize socket info is still opened on deserialize. This should naturally resolve to closed but might show some disconnect errors? Let me know if this behavior is not preferred

@Ltadrian

Ltadrian commented Jul 9, 2026

Copy link
Copy Markdown
Author

Yeah, like James mentioned, I'd repeat that currently as things stand there is a need for more synchronisation with the various socket states (closed/open). But perhaps that's planned since this is still a draft :)

In general though I think this is a good start. I left a few comments about various things I'd improve as well, but nothing major.

@dom96

Thank you for the initial review, I have spent some extra time addressing the feedback and have added a second commit to this merge request to handle the various states I initially put on TODO: halfOpen, opened, closed

I'll re-review my second commit today again but this seems to be functional now

(opened/closed/allowHalfOpen/secureTransport)

Carry the origin socket's runtime state over JS RPC and reconstruct
equivalent behavior on the receiving side.

- Wire format: add secureTransport (new enum), isDefaultFetchPort,
  localAddress,
  and allowHalfOpen to External.socket; fix the socket external doc
  comment.
- opened: track settled state (OpenedState enum); serialize() throws
  DataCloneError unless OPENED; deserialize() resolves it from the
  transferred
  SocketInfo and marks the socket OPENED.
- allowHalfOpen: carried across transfer; when false, re-establish the
  read-EOF -> write-side auto-close on the receiving side.
- closed: extract shared disconnect wiring (used by setupSocket) and
  resolve `closed` on disconnect for transferred sockets.
- deserialize scope: the EOF and closed wiring attach jsg `.then()`
  continuations (illegal under the no-JS deserialize scope), so defer
  them to microtasks; kj-side signals are latched synchronously so
  nothing is missed.
- Tests: allowHalfOpen honored across transfer, unopened transfer
  throws,
  `closed` resolves on close(); js-rpc-test awaits `opened` before
  returning.
@Ltadrian
Ltadrian force-pushed the dlapid/jsrpc_socket branch from b7082f5 to a44dee4 Compare July 9, 2026 19:25
@Ltadrian
Ltadrian marked this pull request as ready for review July 10, 2026 17:34
@Ltadrian
Ltadrian requested review from a team as code owners July 10, 2026 17:34
@codspeed-hq

codspeed-hq Bot commented Jul 10, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 9.77%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 71 untouched benchmarks
⏩ 129 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
bodyWithHeaders[Response] 34.2 µs 31.2 µs +9.77%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing Ltadrian:dlapid/jsrpc_socket (a44dee4) with main (a77b7aa)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Ltadrian
Ltadrian requested review from dom96 and jasnell July 20, 2026 15:39

@dom96 dom96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a test which calls startTls on a socket, otherwise LGTM. Not approving as I'd like James to take a look too.

Comment thread src/workerd/api/sockets.c++
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++
js.v8Isolate->EnqueueMicrotask(js.wrapSimpleFunction(js.v8Context(),
JSG_VISITABLE_LAMBDA((self = socket.addRef(), disconnected = kj::mv(disconnectedOwn)), (self),
(jsg::Lock & js, const v8::FunctionCallbackInfo<v8::Value>& args) mutable {
self.get()->wireClosedToDisconnect(js, kj::mv(*disconnected));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that if these throw JS exceptions, the error will be swallowed entirely by the EnqueueMicrotask

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can happen, for instance, if the microtask queue happens to be drained outside of an active IoContext, for instance. Using EnqueueMicrotask might not be the safest here as opposed to creating a resolved promise and attaching a continuation to it (which utilizes the cross-request promise resolve mechanism)

Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/streams/readable.c++
@Ltadrian
Ltadrian force-pushed the dlapid/jsrpc_socket branch from 2b54fe1 to 4b18d4b Compare July 22, 2026 23:17
- Consolidate disconnect-watcher fulfiller ownership into
  handleDisconnected
- Simplify trackOpenedState/wireClosedToDisconnect GC captures (self =
  JSG_THIS)
- Guard deserialize microtasks with IoContext::hasCurrent()
- Log instead of swallow shutdownWrite failures
- Clarify startTls-on-transferred-socket error message
- Put IoContext first in newNoDeferredProxyReadableStream
- Add transferredSocketCannotStartTls test
@Ltadrian
Ltadrian force-pushed the dlapid/jsrpc_socket branch from 4b18d4b to 1427a28 Compare July 22, 2026 23:18
@Ltadrian

Copy link
Copy Markdown
Author

Addressed most comments by James/Dominik here. Moving entire changeset and discussions to internal MR. Will close for now

@Ltadrian Ltadrian closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants