Skip to content

udp: read one datagram per receive call on an adopted descriptor - #48497

Open
robobun wants to merge 2 commits into
mainfrom
robobun/e54e4e0d/udp-adopted-fd-one-datagram
Open

robobun wants to merge 2 commits into
mainfrom
robobun/e54e4e0d/udp-adopted-fd-one-datagram

Conversation

@robobun

@robobun robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A UDP descriptor that Bun adopts can be one that another process also reads. child.send(message, socket) delivers a socket to bind({ fd, exclusive: true }) (src/js/builtins/Ipc.ts:98).
  • An adopted socket takes up to 8 datagrams per recvmmsg (packages/bun-usockets/src/loop.c:1100). When a 'message' listener calls close(), on_data (src/runtime/socket/udp_socket.rs:180) drops the rest. With 32 queued, the other process gets 24 (Node v26.3.0: 31).

Fix

  • us_create_udp_socket_from_fd loses its shared parameter. Every adopted descriptor reads one datagram per receive call, as Node does.
  • The private sharedFd option and UDPSocketConfig.shared_fd are deleted. Sockets that Bun creates keep the batch.
  • Verified: 3 new cases in test/js/bun/udp/dgram.test.ts, all fail on main. Notes lists the other suites.
  • Self-reviewed: 33 concerns raised on a wider first draft, all addressed, most of them by this split.

Background

Downsides

Notes

No issue reports this loss. It was found by a read of the code, then confirmed against Node v26.3.0 with the script in the status comment.

What two processes see. 32 datagrams are queued on one UDP socket that two processes hold. One process stops in its first 'message' listener. The table gives the count of datagrams that the other process then receives.

how the processes share the socket which one stops Node main this PR
child.send(message, socket) the receiver, close() 31 24 31
child.send(message, socket) the receiver, process.exit() 31 24 31
inherited descriptor, bind({ fd }) the child, close() 31 24 31
inherited descriptor, Bun.udpSocket({ fd }) the child, close() (no such API) 24 31
child.send(message, socket) the sender, close() 31 24 24
child.send(message, socket) and close() in one listener the sender 31 24 24

The last two rows are the sender of an IPC send. Its socket is one that Bun created, so this PR does not change it.

History of the flag. All four commits are on the branch of #31829.

  • 1f5f6c5 (2026-06-05) made every adopted descriptor read one datagram per call.
  • 90d9e6c (2026-07-02) kept that rule. Its message: a descriptor from a user "may be shared with another process just as well, so the conservative one-datagram per-recvmmsg behavior applies there too". Its comment on shared_fd: "Receive one datagram per syscall so a close() from the data callback cannot discard batched packets."
  • 3be0485 (2026-07-09, "address second-pass review findings") added the shared parameter. Its message: "standalone dgram.bind({fd}) and Bun.udpSocket({fd}) keep the recvmmsg batch".
  • b7a6441 (2026-08-04) added the receive of a dgram.Socket over IPC. It adopts with bind({ fd, exclusive: true }), so it got the batch.

What Node does. libuv reads one datagram per recvmsg and checks the handle again before the next one (uv__udp_recvmsg, deps/uv/src/unix/udp.c). UV_UDP_RECVMMSG is an opt-in flag of uv_udp_init_ex. Node's src/udp_wrap.cc calls uv_udp_init, so Node never batches.

Measurements. Linux x64, release builds of af566a3 with and without this change. Node is v26.3.0.

  • Receive calls per 100 readable events, K datagrams queued per event, K = 1 / 2 / 8 / 32 / 192. bind({ fd }) and Bun.udpSocket({ fd }) on main: 200 / 200 / 200 / 500 / 2500. With this PR: 200 / 300 / 900 / 3300 / 19300. Node on a socket that it creates: 200 / 300 / 900 / 3200 / 19200.
  • Receive phase of one burst of K datagrams of 64 bytes: the time from the last send() to the K-th 'message'. One process sends and receives. Median of 7 interleaved runs of 20000 bursts on one pinned CPU.
K bind({ fd }) main this PR delta Bun.udpSocket({ fd }) main this PR delta
1 1040 ns 970 ns -6.7% 1000 ns 930 ns -7.0%
2 1560 1700 +9.0% 1480 1630 +10.1%
4 2380 3170 +33.2% 2280 2970 +30.3%
8 3990 6030 +51.1% 3760 5760 +53.2%
64 32990 49600 +50.3% 30480 46120 +51.3%
  • Noise floor: main against main gives +0.8% at K=8 and +0.5% at K=64.
  • The whole burst, send() calls included: +16% at K=8 and at K=64.
  • Node on a socket that it creates, same harness: 1950 / 3660 / 6840 / 13310 / 106240 ns at K = 1 / 2 / 4 / 8 / 64.
  • Sockets that Bun creates, same harness, main against this PR: bind(port) -0.9% / -0.4% / -1.8% and Bun.udpSocket({ port }) +1.9% / -1.2% / -0.4% at K = 1 / 8 / 64. That is noise.
  • Instructions per datagram (user and kernel), bind({ fd }), one process sends and receives, 6 interleaved pairs: 22289 to 21558 at K=1 (-3.3%), 14897 to 15685 at K=8 (+5.3%), 14328 to 15120 at K=32 (+5.5%). Bun.udpSocket({ fd }): -3.5% at K=1, +5.3% at K=8, +5.7% at K=32.
  • Binary: text + data goes from 138056209 to 138055554 bytes (-655).
  • The container has no perf, valgrind or strace. The call counts come from an LD_PRELOAD counter of recvmmsg and recvmsg. The instruction counts come from perf_event_open.

Other platforms.

  • macOS below 15.6 has no batched receive call. The batch was a loop of recvmsg there, so this PR adds no receive call on it. Not measured: no macOS machine here.
  • Windows: the arm of bsd_recvmmsg ignored the count and always took up to 8 datagrams. It now honours the count, as the macOS loop does. Not run: no Windows machine here. bind({ fd }) refuses every descriptor on Windows, so only Bun.udpSocket({ fd }) reaches that arm with a count of 1.

Tests.

  • A sender queues 32 datagrams and end on a bound descriptor. Then a socket adopts the descriptor in one of three ways: bind({ fd }), bind({ fd, exclusive: true }), Bun.udpSocket({ fd }). The first listener reads the rest of the queue through the descriptor, then calls close(). On main the listener finds 24 datagrams, because the socket already took 8.
  • The cases run in one process and take milliseconds. They are skipped on Windows, where bind({ fd }) refuses every descriptor.
  • Suites run on a debug build: test/js/bun/udp/dgram.test.ts (65 pass), udp_socket.test.ts (234), udp_socket_recv_flags.test.ts (2), test/js/node/dgram/node-dgram.test.js (7), test/js/node/child_process/child_process_ipc_handle.test.ts (15), and the 89 files under test/js/node/test/{parallel,sequential} with dgram or udp in the name. test/internal/builtins-table/ (149 pass) ran with a release build.

Other PRs. #48393 restructures UDPSocketConfig.shared_fd. This PR deletes that field.

A UDP descriptor that Bun adopts can be one that another process also
reads: child.send(message, socket) delivers a socket to
bind({ fd, exclusive: true }). Such a socket took up to 8 datagrams per
recvmmsg. When a 'message' listener closed it, it dropped the ones it
had not delivered, and the other process never got them.

us_create_udp_socket_from_fd loses its `shared` parameter. Every adopted
descriptor reads one datagram per receive call, as Node does. The
private sharedFd option of Bun.udpSocket, its builtin name and
UDPSocketConfig.shared_fd are deleted.

Sockets that Bun creates keep the batch. That includes the sender of
child.send(message, socket), which this change does not cover.

The Windows arm of bsd_recvmmsg ignored the count. It now honours it, as
the other arms do.
@robobun

robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:41 PM PT - Nov 10th, 2026

❌ @robobun, your commit a1d8b88 has 1 failures in Build #139863 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 48497

That installs a local version of the PR into your bun-48497 executable, so you can run:

bun-48497 --bun

@robobun

robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduction: two child processes get one dgram.Socket over IPC. 32 datagrams are queued while neither reads. One child calls close() in its first 'message' listener. The other child reads what is left.

closer counter
Node v26.3.0 1 31
main 1 24 (datagrams 1 to 7 are lost)
this PR 1 31
Script (run it with node or bun)
// Two children get one UDP socket over IPC. 32 datagrams are queued while neither reads.
// The closer calls close() in its first 'message' listener. The counter reads what is left.
const dgram = require("node:dgram");
const { fork } = require("node:child_process");
const { readSync } = require("node:fs");

const role = process.argv[2];
if (role) {
  process.once("message", (_, socket) => {
    const got = [];
    socket.on("message", data => {
      const text = String(data);
      if (role === "counter" && text !== "end") return void got.push(text);
      if (role === "closer") got.push(text);
      socket.close();
      process.send(got, () => process.exit());
    });
    // Read nothing until the parent writes to stdin.
    process.send("ready", () => readSync(0, Buffer.alloc(1)));
  });
} else {
  const socket = dgram.createSocket("udp4");
  const sender = dgram.createSocket("udp4");
  socket.bind(0, "127.0.0.1", async () => {
    const port = socket.address().port;
    const start = role => {
      const child = fork(__filename, [role], { stdio: ["pipe", "inherit", "inherit", "ipc"] });
      const ready = Promise.withResolvers(), report = Promise.withResolvers();
      child.on("message", message => (message === "ready" ? ready : report).resolve(message));
      child.send("socket", socket);
      return { child, ready: ready.promise, report: report.promise };
    };
    const closer = start("closer"), counter = start("counter");
    await Promise.all([closer.ready, counter.ready]);
    socket.close();
    await new Promise(resolve => sender.connect(port, "127.0.0.1", resolve));
    for (let i = 0; i < 32; i++) sender.send(String(i));
    closer.child.stdin.write("go");
    const closed = await closer.report;
    sender.send("end");
    counter.child.stdin.write("go");
    const counted = await counter.report;
    sender.close();
    console.log(`closer got ${closed.length}, counter got ${counted.length}: ${counted[0]} to ${counted.at(-1)}`);
  });
}

The sender of an IPC send is a different socket. #48498 covers it.

CI: the diff is green on build 139863. The one red job is test/aot/parallel/what-a-build-takes-grows-with-its-program/values-kept-by-an-async-function-one-more-across-each-await.js on ubuntu 25.04 aarch64. It checks the memory of an AOT build (135 MB against a limit of 133 MB). This PR does not touch that code.

@claude claude Bot 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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked the mechanical parts: the Rust extern for us_create_udp_socket_from_fd in src/uws_sys/udp.rs matches the C definition parameter-for-parameter after the shared removal, and no other caller remains; the remaining sharedFd hits in net.ts/dgram.ts/cluster are the public handle property, not the deleted private builtin name; the Windows bsd_recvmmsg hunk is behavior-preserving for the only prior caller (max_packets == LIBUS_UDP_RECV_COUNT) and returns 1 correctly for the new shared case; the SO_REUSEADDR claim in the libusockets.h comment matches bsd_prepare_adopted_udp_socket. Not run here: the new tests, bun run rust:check-all, and the mordant lint whose baseline entry was removed.

Extended reasoning...

The change removes the shared flag from the adopted-UDP-descriptor path so every bind({ fd }) / Bun.udpSocket({ fd }) socket reads one datagram per receive call, deletes the private sharedFd builtin name end-to-end (C, Rust FFI, bindings.cpp, builtin tables, dgram.ts), fixes the Windows bsd_recvmmsg loop to honour max_packets, and adds three POSIX test cases. It touches no security-sensitive surface. The plumbing is consistent across all consumers, but the PR deliberately reverts a prior design decision (batching on standalone adopted descriptors) with a measured ~50% receive-phase cost at K=8 and asks named maintainers to confirm, so a human should weigh that trade-off; the inline findings already signal that.

Comment thread test/js/bun/udp/dgram.test.ts Resolved
Comment thread test/js/bun/udp/dgram.test.ts Resolved
Comment thread packages/bun-usockets/src/udp.c Resolved
Comment thread packages/bun-usockets/src/udp.c Resolved
@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The change removes the sharedFd option and parameter from UDP descriptor adoption. Adopted sockets now set shared descriptor handling unconditionally. The Windows receive loop is bounded by max_packets, and POSIX tests check datagram reads from adopted descriptors.

Changes

UDP descriptor adoption

Layer / File(s) Summary
Remove shared-descriptor option and parameter
src/codegen/builtins/host/globals.builtins.ts, src/js/builtins/BunBuiltinNames.h, src/jsc/bindings/bindings.cpp, src/jsc/lib.rs, src/js/node/dgram.ts, src/runtime/socket/udp_socket.rs, src/uws_sys/udp.rs, packages/bun-usockets/src/udp.c, mordant-baseline.toml
The sharedFd option and the runtime configuration field are removed. The Rust and C descriptor-adoption functions no longer accept a shared argument. Adopted UDP sockets set shared_fd to 1 unconditionally.
Update adopted-descriptor receive handling
packages/bun-usockets/src/bsd.c, packages/bun-usockets/src/internal/internal.h, packages/bun-usockets/src/libusockets.h, test/js/bun/udp/dgram.test.ts
The Windows receive loop uses max_packets. Comments and documentation describe one-datagram receive calls. POSIX tests check the order of datagrams read from adopted descriptors.
Merge Risk: Minimal · up to a1d8b

The adopted-descriptor receive path follows the intended one-datagram limit. No actionable merge-blocking risk remains after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly summarizes the main change: adopted UDP descriptors read one datagram per receive call.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description explains the problem, the fix, and how the author verified it. It includes test results and performance tradeoffs. It uses different headings from the repository template, but the requ…
Full details: Description check

Explanation

The description explains the problem, the fix, and how the author verified it. It includes test results and performance tradeoffs. It uses different headings from the repository template, but the required information is present.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between b00b9f6 and a1d8b88.

Files selected for processing: 1

  • test/js/bun/udp/dgram.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

The raw handle, the sender and the adopted socket go on a DisposableStack
as soon as they exist. The 'error' event of the dgram socket and the
error handler of Bun.udpSocket reject the promise that the case awaits.

The datagrams are queued before the socket adopts the descriptor, so the
case does not wait for 'listening'.

@claude claude Bot 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.

I re-reviewed after a1d8b88 and found no bugs; the follow-up resolves both of my earlier test notes (resources go on a DisposableStack before the assertions, and both the dgram 'error' event and the Bun.udpSocket error handler now reject the awaited promise). Because this reverses a deliberate choice from #31829 and trades up to ~50% on the receive phase of bursts on bind({ fd }) / Bun.udpSocket({ fd }) for one-datagram-per-call semantics, a maintainer should still confirm that tradeoff.

What was reviewed:

  • Signature change of us_create_udp_socket_from_fd: diffed the C definition, the header, and the Rust extern + wrapper parameter-by-parameter; grepped for other callers (none, including cfg-gated code).
  • Removal of the sharedFd private name across BunBuiltinNames.h, bindings.cpp enum/switch, src/jsc/lib.rs, globals.builtins.ts and dgram.ts; the remaining sharedFd hits in net.ts/dgram.ts/cluster are the unrelated public handle property.
  • Windows bsd_recvmmsg now honours max_packets; the early-return-on-would-block path and loop.c's do/while(!closed) drain are unchanged and consistent with the new count.
  • Not run here: the new test cases and bun run rust:check-all (cross-platform change).
Extended reasoning...

The diff removes the shared parameter from us_create_udp_socket_from_fd so every adopted UDP descriptor reads one datagram per receive call, deletes the private sharedFd option from five mirror sites plus the Rust config field, fixes the Windows bsd_recvmmsg arm to honour max_packets, and adds three test.each cases in test/js/bun/udp/dgram.test.ts. It touches no security-sensitive surface (no auth, crypto, or untrusted parsing). The code is small and mechanically consistent, and the follow-up commit addressed the two inline nits from the prior review. Defer rather than approve because the change deliberately reverses a prior maintainer decision and carries a measured performance cost on adopted-descriptor bursts, which is a design tradeoff a human should sign off on; no CODEOWNER covers the changed paths and no third-party objection is outstanding.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant