Skip to content

feature: Implement basic UDP support (#1630) - #6792

Closed
tarruda wants to merge 7 commits into
oven-sh:mainfrom
tarruda:usockets-udp
Closed

tarruda wants to merge 7 commits into
oven-sh:mainfrom
tarruda:usockets-udp

Conversation

@tarruda

@tarruda tarruda commented Oct 30, 2023 •

Copy link
Copy Markdown

What does this PR do?

#1630

This implements a simple UDP programming API on the Bun namespace. It is still not fully optimized, but should be enough to implement a node.js dgram shim (in a future PR).

  • Documentation or TypeScript types
  • Code changes

How did you verify your code works?

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I or my editor ran zig fmt on the changed files
  • I included a test for the new code, or an existing test covers it (TODO)
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
  • I added TypeScript types for the new methods, getters, or setters

- Get the peer address correctly
- Implement us_udp_socket_close
- Remove printf statement when creating udp socket

Signed-off-by: Thiago Padilha <thiago@padilha.cc>
Also fix Poll.deinit to receive a loop pointer.

Signed-off-by: Thiago Padilha <thiago@padilha.cc>
For now this only contains a basic implementation, but it should be
enough to implement a node.js shim

Signed-off-by: Thiago Padilha <thiago@padilha.cc>
@liz3

liz3 commented Oct 30, 2023

Copy link
Copy Markdown
Contributor

Pr seams like a good idea but api wise we might want to integrate it into Bun.connect rather then a standalone function maybe, it might also maybe allow to reuse some code, although thats not mandatory i think but i think it def makes sense to integrate it as a option rather then a standalone api on the Bun global object.

@tarruda
tarruda marked this pull request as draft October 30, 2023 10:07
@tarruda

tarruda commented Oct 30, 2023

Copy link
Copy Markdown
Author

Pr seams like a good idea but api wise we might want to integrate it into Bun.connect rather then a standalone function maybe, it might also maybe allow to reuse some code, although thats not mandatory i think but i think it def makes sense to integrate it as a option rather then a standalone api on the Bun global object.

That might be possible, but note that UDP has a different programming model than TCP:

  • There's a limitation with the packet size for UDP (you can't send an arbitrary blob as a a packet).
  • From a syscall API POV, there's not much difference between listen/connect with UDP. Each side is binding to an interface/port and can send packets to any address. This is why the connect()~ method is optional and simply sets a default address for future calls of send()`.
  • While connect() system call can be used in UDP sockets, uSockets UDP API doesn't seem to expose that option (and it uses a syscall that assumes the socket is unconnected).

Signed-off-by: Thiago Padilha <thiago@padilha.cc>
It is not necessary to store these fields since they are only used
during socket creation.

Signed-off-by: Thiago Padilha <thiago@padilha.cc>
The previous code could result in non 0-terminated strings, fix that by
using `sliceZBuf` which can write to a stack buffer. This is OK because
we have a predictable max length of an ip address.

Signed-off-by: Thiago Padilha <thiago@padilha.cc>
@liz3

liz3 commented Oct 31, 2023 •

Copy link
Copy Markdown
Contributor

Pr seams like a good idea but api wise we might want to integrate it into Bun.connect rather then a standalone function maybe, it might also maybe allow to reuse some code, although thats not mandatory i think but i think it def makes sense to integrate it as a option rather then a standalone api on the Bun global object.

That might be possible, but note that UDP has a different programming model than TCP:

* There's a limitation with the packet size for UDP (you can't send an arbitrary blob as a a packet).

* From a syscall API POV, there's not much difference between listen/connect with UDP. Each side is binding to an interface/port and can send packets to any address. This is why the `connect()~ method is optional and simply sets a default address for future calls of `send()`.

* While `connect()` system call can be used in UDP sockets, uSockets UDP API doesn't seem to expose that option (and it uses a syscall that assumes the socket is unconnected).

These seam to be mostly implementation details though, correct me if wrong but the user-facing js api does not need to really change for socket clients right, which is from where the idea came.

@tarruda

tarruda commented Oct 31, 2023

Copy link
Copy Markdown
Author

These seam to be mostly implementation details though, correct me if wrong but the user-facing js api does not need to really change for socket clients right, which is wrong where the idea came.

It is not an implementation detail, it is simply a lower level protocol than TCP.

When you send a UDP packet over the wire, it is not guaranteed the other end will receive it and the sender gets no confirmation. It is up to the application to deal with it, which is why UDP is more useful in real time chat apps and games. The concept of "streams" is a higher level abstraction created by TCP (upon which other protocols such as HTTP are built upon). When you send some data over a TCP connection, the other end is guaranteed to receive it because the TCP layer will resend packets which were not received.

This is not something we can abstract away from users without reimplementing TCP and incurring significant performance loss (At which point the user would have no reason to use UDP in the first place).

Here's some extra reading if you want to understand more: https://www.pluralsight.com/blog/it-ops/networking-basics-tcp-udp-tcpip-osi-models

@liz3

liz3 commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

Im aware what UDP is and i in no point suggested that its needed to reimplement TCP over udp?
My point is that the user facing api like onData/write stay the same? These functions dont change, if you look at the events emited by node the core api concepts user facing dont change, https://nodejs.org/api/dgram.html
But i never meant to say its the implementations responsibility to ensure delivery?

@liz3

liz3 commented Oct 31, 2023 •

Copy link
Copy Markdown
Contributor
const client = Bun.connect({
  hostname: "localhost",
  port: 41234,
  protocol: "udp",
  socket: {
    open(socket) {}, // can be auto emitted
    close(socket) {}, // can be emitted after the user calls client.close() or omitted
    error(socket, error) {}, // same as data but for errors
  },
});


const server = await Bun.listen({ // the await can be akain to the node listening event
  hostname: "localhost",
  port: 41234,
  protocol: "udp",
  socket: {
    data(rinfo, data) {}, // message received from client
    error(error) {}, // error handler
  },
});

This does not need to be final or complete but maybe it illustrates the idea better.

@tarruda

tarruda commented Oct 31, 2023

Copy link
Copy Markdown
Author

It is possible to implement the way you suggest, but IMO it is going to be a forced consistency with the TCP constructors, and the semantics will be different:

  • Many of the options supported by connect/listen on TCP side will not be supported with UDP (tls, connectionError, timeout won't make sense for UDP).
  • The object returned by both listen/connect will have a different API than the TCP sockets.

Also, the fact that I'm providing a base API with a higher level means it will be more difficult to implement a node.js shim on top of it. For example, Node.js also doesn't make a distinction between client and server udp sockets (because there's no underlying distinction), and it is perfectly fine for a "server" to "connect" to another address/port.

The API I've implemented here closely follows the uSockets API

  const socket = Bun.createUdpSocket({    
     bind: {    
      address: '127.0.0.1',    
      port: 5678,    
    },    
    socket: {    
      data(packet, peer, port) {    
        // echo
        this.send(packet, peer, port);    
      }    
    }    
  }); 

uSockets public API is also not ideal because it binds the socket immediatelly and requires you to specify an address and port. The abstraction I've made here is that if you don't specify bind address or port, it will default to "0.0.0.0" for address(because we want to have a global return address) and 0 for port (which means the OS will select a random port). We can modify uSockets, but so far I'm taking @paperdave suggestion to keep uSockets changes to a minimum.

@dpnova

dpnova commented Nov 2, 2023

Copy link
Copy Markdown

"connect" is a TCP concept - so it makes no sense in the context of UDP.

@liz3

liz3 commented Nov 2, 2023 •

Copy link
Copy Markdown
Contributor

"connect" is a TCP concept - so it makes no sense in the context of UDP.

https://nodejs.org/api/dgram.html#socketconnectport-address-callback can also refer to association, it does not need to be solely related to tcp.
Which is also why in my example the Bun.connect has no await. because theres no network involved first hand.

@tarruda

tarruda commented Nov 2, 2023

Copy link
Copy Markdown
Author

"connect" is a TCP concept - so it makes no sense in the context of UDP.

https://nodejs.org/api/dgram.html#socketconnectport-address-callback can also refer to association, it does not need to be solely related to tcp. Which is also why in my example the Bun.connect has no await. because theres no network involved first hand.

Right now, neither connect nor send do name resolution because I wanted to make Bun UDP API as close to uSockets as possible without implicit calls (I imagine UDP use cases need bare metal performance as possible). In order to make these functions invoke DNS automatically, they need to be async.

I'm going to wait for feedback from maintainers before making any extra changes though.

@evsar3 evsar3 mentioned this pull request Nov 7, 2023
@paperclover

Copy link
Copy Markdown
Contributor

heads up, seems this doesnt compile on MacOS right now, potential zig bug ziglang/translate-c#122

you can test a zig compile for macos target with
zig build obj -Dtarget=aarch64-macos-none -Dgenerated-code=build/codegen

@@ -0,0 +1,477 @@
const bun = @import("root").bun;
const std = @import("std");
const c = @cImport({

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.

this is probably the root of the macos compilation error. an alternative to this is hand writing the extern fn stuff.

@Electroid Electroid mentioned this pull request Nov 22, 2023
3 of 6 tasks
@tarruda tarruda closed this Nov 22, 2023
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.

4 participants