Implement inbound UDP handler - #7130
Conversation
48e6d49 to
1011f25
Compare
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
ed9a31d to
a6ef39c
Compare
|
|
||
| // Resolves with the next inbound datagram, or kj::none once the flow has ended (e.g. an idle | ||
| // timeout). Must not be called again after resolving kj::none, and must not have more than one | ||
| // outstanding call at a time. |
There was a problem hiding this comment.
How is back pressure handled? Specifically, if we're receiving datagrams faster than calls to receive() .. what happens?
| virtual kj::Promise<kj::Maybe<kj::Array<kj::byte>>> receive() = 0; | ||
|
|
||
| // Sends one outbound datagram to the peer. | ||
| virtual kj::Promise<void> send(kj::ArrayPtr<const kj::byte> datagram) = 0; |
There was a problem hiding this comment.
kj::Promise<void>... implies that these aren't send-and-forget. What is the resolve criteria here?
| JSG_READONLY_PROTOTYPE_PROPERTY(opened, getOpened); | ||
| JSG_READONLY_PROTOTYPE_PROPERTY(upgraded, getUpgraded); | ||
| JSG_READONLY_PROTOTYPE_PROPERTY(secureTransport, getSecureTransport); | ||
| JSG_READONLY_PROTOTYPE_PROPERTY(protocol, getProtocol); |
There was a problem hiding this comment.
Just note that any additional properties that are not part of the proposed standard spec should be marked as such.
| } | ||
|
|
||
| // A WritableStreamSink that sends each JS write() call as exactly one outbound datagram. Unlike a | ||
| // byte-stream sink, there is no buffering: one write() call is one DatagramChannel::send() call, |
There was a problem hiding this comment.
What does "there is no buffering" mean here? The WritableStream controller still has it's own buffering.
There was a problem hiding this comment.
Apologies, I misunderstood how internal streams worked, I thought there was no buffering done for them, and TCP's sink was the one doing it.
|
|
||
| kj::Promise<void> write(kj::ArrayPtr<const kj::byte> buffer) override { | ||
| return channel->send(buffer); | ||
| } |
There was a problem hiding this comment.
hmm.. this might end up being a bit surprising to users. The behavior here needs to be thought through. Specifically, take something like:
const rs = new ReadableStream({ ... });
await rs.pipeTo(socket.writable);With a TCP socket, this is fine, regular stream semantics.
With this, doh, everything is a separate datagram. Each packet may be of a different size, might split surrogate pair bytes, might split utf8 bytes, etc. I'm not convinced it's a great idea to re-use the Socket in this way. Not going to block but I'm far from convinced.
| jsg::Ref<ReadableStream> newDatagramReadableStream(jsg::Lock& js, kj::Rc<DatagramChannel> channel) { | ||
| auto controller = newReadableStreamJsController(); | ||
| auto stream = js.allocAccounted<ReadableStream>( | ||
| sizeof(ReadableStream) + controller->jsgGetMemorySelfSize(), kj::mv(controller)); |
There was a problem hiding this comment.
this needs to use the JsReadableStream abstraction for creating the ReadableStream or this breaks under the TS streams work.
There was a problem hiding this comment.
Done! I'll let you resolve this comment if my changes are good
| JSG_FAIL_REQUIRE(Error, "Handler does not export a connect() function."); | ||
| } | ||
|
|
||
| kj::Promise<void> ServiceWorkerGlobalScope::connectUdp(kj::String host, |
There was a problem hiding this comment.
I'm definitely not a fan of introducing a new non-standard global. connect(...) is one thing because we have a standards-track spec behind it.
There was a problem hiding this comment.
by "new global" do you mean new handler function exposed to customers ? My current goal was to reuse the existing connect() handler. I updated the description with a code usage example to show that
Does this change your comment ?
dom96
left a comment
There was a problem hiding this comment.
I would ideally like to see this added to the sockets spec before it is implemented: https://sockets-api.proposal.wintertc.org/
It's also worth considering how our design compares to the direct-socket API. Ideally we shouldn't diverge from it unless it's necessary. It would be nice to reuse its UDPMessage + ReadableStream/WritableStream approach for example.
| readonly readable: unknown; | ||
| readonly writable: unknown; | ||
| readonly closed: Promise<void>; | ||
| readonly protocol: 'tcp' | 'udp'; |
There was a problem hiding this comment.
Worth noting that the direct-sockets spec proposal defines separate types: a TCPSocket and a UDPSocket. https://wicg.github.io/direct-sockets/#udpsocket-interface
It may be a good idea for us to do the same. The fact that the UDP socket doesn't support TLS nor startTls (plus a bunch of other methods that work on Socket) seems to suggest that it shouldn't reuse Socket.
There was a problem hiding this comment.
I'm not sure I understand. Were you asking about exposing a Socket object for TCP and a UDPSocket object for UDP ?
There was a problem hiding this comment.
Yeah, exactly. Curious what @jasnell thinks on this though.
a6ef39c to
f6d23ca
Compare
Summary
This PR adds support for handling inbound UDP connections. It does 2 things:
protocolfield to theSocketobject used in connect() handlers, following [wrangler] addconnectwrangler config section workers-sdk#14995The UDP connect() handler has been implemented as a custom event because connect() required a
kj::AsyncIoStreamwhich is byte-oriented rather than message-oriented like UDP requiresThese commits can be split into 2 PRs if necessary
Example usage
testable by adding this bit in a workerd config:
sockets = [ ( name = "http", address = "*:8787", http = (), service = "main" ), ( name = "tcp", address = "*:5432", tcp = (), service = "main" ), ( name = "udp", address = "*:5599", udp = (idleTimeoutMs = 30000), service = "main" ), ]