Repository navigation
Conversation
WalkthroughReplaces async UDP socket creation with a synchronous Rust bridge, adds implicit bind for multicast membership APIs, and makes ChangesUDP synchronous bind, implicit multicast bind, and dns.lookup callback rethrow
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:39 AM PT - Jun 30th, 2026
❌ @robobun, your commit badb9e8 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33031That installs a local version of the PR into your bun-33031 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/udp/dgram.test.ts`:
- Around line 382-389: The test in dgram.test.ts leaves the udp4 socket open if
the addMembership assertion fails, so register cleanup before the assertion
path. Update the bind/await flow in the a bind() already in flight still throws
ERR_SOCKET_DGRAM_NOT_RUNNING test to use try/finally or using so socket.close()
always runs, and keep the cleanup tied to the socket created by createSocket and
the Promise.withResolvers listening flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07a25be3-1ce1-4639-a004-9e7cc74ee270
📒 Files selected for processing (6)
src/js/node/dgram.tssrc/js/node/dns.tssrc/runtime/socket/udp_socket.rstest/js/bun/udp/dgram.test.tstest/js/bun/udp/sendMany-reentrancy-fixture.tstest/js/node/dns/dns-lookup-keepalive.test.ts
|
Status: the diff is complete, every review thread on this PR is resolved, and the remaining CI red is not caused by this change. Note Updated after a rebase. The branch has since been rebased onto At build 66640 (sha 8de7caa), 278 test jobs passed and 4 lanes were red. None of the 4 failures touch anything in this diff (
Every subsequent push has hit the same classes of unrelated red lanes. At build 66651 (sha bf17eed, two further test-only commits from bot review feedback: 282 passed, 4 failed), the only genuine test failure in the whole build was the alpine I then spent the one Those last three test failures need no further analysis. dff22bd is an empty commit, so build 66701 built a tree byte-identical to build 66651's, and all three were green on the same The source change has been stable since 6070f18; everything after it is test-only review feedback plus that one empty retrigger, and all 7 review findings from claude and CodeRabbit are addressed with every thread resolved. There will be no further pushes from me. This is ready for a maintainer: the diff is green on every lane that actually ran its code, and the remaining red is the infrastructure failure and ambient flake documented above. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/udp/dgram.test.ts`:
- Around line 349-352: The retry loop in sendRec ignores async failures from
sender.send(), so a broken implicit-bind/send path can keep rescheduling until
timeout. Update sendRec to inspect the callback error from sender.send and
stop/reject the test immediately when it is non-null, instead of always
scheduling the next retry. Use the existing sendRec and sender.send flow in the
UDP dgram test to wire the failure path through the callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67015cc7-93a6-454b-a3e7-0bfdb0e57e30
📒 Files selected for processing (2)
src/js/node/dns.tstest/js/bun/udp/dgram.test.ts
addMembership(), dropMembership(), addSourceSpecificMembership(), and
dropSourceSpecificMembership() on an unbound socket threw
ERR_SOCKET_DGRAM_NOT_RUNNING. Node implicitly binds the socket to a random
port first (libuv's uv__udp_maybe_deferred_bind) and then applies the
membership change.
The `this.bind({port: 0})` call that #16446 added for this was placed after
the not-running throw, so it never ran; and Socket.prototype.bind is
asynchronous (DNS lookup + promise), which a synchronous membership call
cannot wait on.
Bun.udpSocket already creates and binds its socket synchronously; the Promise
it returns is only API shape. Split that body into UDPSocket::create and
expose it to the dgram builtin as `UDPSocket.jsCreate`, so the implicit bind
can happen inline. Socket.prototype.bind now goes through the same helper,
which removes the extra microtask hop between the address lookup resolving
and `listening` firing; with a synchronous custom `lookup` option, `bind()`
now emits `listening` before it returns, exactly as Node does. The
sendMany reentrancy fixture relied on that extra hop, so it registers its
`listening` listeners before calling bind().
dns.lookup()'s callback ran inside the `.then` of a `.then(...).catch(...)` chain, so a throw from the callback fell through to the chained `.catch`, which invoked the callback a second time with the callback's own exception presented as the lookup error. Node invokes the callback exactly once and the throw surfaces as an uncaughtException. This became observable through node:dgram after the previous commit removed the extra microtask between bind()'s address lookup resolving and "listening" being emitted: a throwing "listening" listener re-entered bind()'s lookup callback as a lookup error, which reset bindState to UNBOUND while state.handle.socket still held the live native socket, so the next send() or bind() silently created and leaked a second socket. Invoke the callback from process.nextTick in both arms of the chain, the same idiom lookup() already uses for its IP-literal fast path. A throw from the callback is then an uncaughtException, exactly as in node, and can never reach the `.catch`. throwIfEmpty() still relies on the chained `.catch`, so the two-argument `.then(onFulfilled, onRejected)` form the rest of node:dns already uses would not have worked here.
The two new spawned-process tests set `stderr: "pipe"` without ever reading
it, so an unexpected child failure would have surfaced as an opaque
`{stdout: "", exitCode: 1}` with the diagnostic dropped. Drain it and
include it in the asserted object, filtering the benign ASAN startup
warning the same way the neighboring udp_socket.test.ts does.
Also point the "see also" comment at dns-lookup-keepalive.test.ts, where
that test actually lives.
The previous commit routed lookup()'s callback through process.nextTick so
a throw from it could not reach the chained `.catch` and invoke the
callback a second time. That deferral regressed
test/js/node/test/parallel/test-net-connect-memleak.js on the alpine CI
lanes, and reproduces 30/30 on an unmodified main build just by adding a
process.nextTick (or an extra promise reaction) to the net.connect lookup
path: the test exercises conservative GC, `net.connect(port)` resolves
"localhost" through dns.lookup, and any added deferral in that path keeps
the once('connect') listener's closure reachable across the test's gc().
Invoke the callback synchronously from the same `.then` reaction it has
always run in, so the success path is structurally unchanged, and wrap
only the invocation in a try/catch that re-raises the callback's own throw
from a queueMicrotask. That surfaces it as an uncaughtException exactly as
node does and keeps it out of the chained `.catch`. The error arm gets the
same treatment so a throwing error callback is an uncaughtException there
too, rather than an unhandled rejection of the chain's terminal promise.
The bind-in-flight test left its socket open if the addMembership assertion failed. Register the cleanup before the assertions like the rest of the describe block.
"the implicitly bound socket can send" was the only send-then-receive test in test/js/bun/udp/ that fired a single datagram and then awaited the receive unconditionally; a drop over loopback on a loaded host would hang it to the per-file timeout. Drive it with the same sendRec retry loop the other send tests in this file use.
dgram's send() reports errors only through its callback when one is supplied, never as an "error" event, so the retry loop was ignoring them and would have spun until the file timeout. Reject the awaited promise with the real error instead.
dff22bd to
badb9e8
Compare
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-30, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Problem
Calling
addMembership()on anode:dgramsocket beforebind()throwsERR_SOCKET_DGRAM_NOT_RUNNINGin Bun. Node implicitly binds the socket to a random port first (libuv'suv__udp_maybe_deferred_bind) and then joins the group, as documented:The same applies to
dropMembership,addSourceSpecificMembership, anddropSourceSpecificMembership, which all go through the same libuv deferred bind.Cause
dgram.ts's membership functions throwERR_SOCKET_DGRAM_NOT_RUNNINGwheneverhandle.socketis missing. #16446 added athis.bind({ port: 0, exclusive: true })for the unbound case, but placed it after that throw, so it was unreachable. It could not have worked anyway:Socket.prototype.bindis asynchronous (address lookup, then a.thenon theBun.udpSocket()promise), and Node's membership operations are synchronous, so there is nothing for them to wait on.Fix
Bun.udpSocket()already creates and binds the uws socket synchronously; the Promise it returns is only the public API shape. This splits that body into aUDPSocket::createthat returns the socket directly and exposes it to the dgram builtin asUDPSocket.jsCreate. A newimplicitBind()helper calls it when a membership function is invoked on an unbound socket, binding to0.0.0.0:0(or[::]:0) synchronously, exactly like libuv's deferred bind.Socket.prototype.bindnow goes through the sameattachSocket()helper instead ofBun.udpSocket(...).then(...), so there is a single place that creates the handle's socket. That also removes an extra microtask hop between the address lookup resolving andlisteningfiring, matching Node: Node emitslisteningsynchronously from inside the lookup callback. The only place the hop was observable is with a synchronous customlookupoption; thesendManyreentrancy fixture relied on it and now registers itslisteninglisteners before callingbind()(the reentrancy assertions it exists for are unchanged).Behavior preserved:
bind()still in flight, still throwsERR_SOCKET_DGRAM_NOT_RUNNING.addMembership EINVALwithout binding (Node validates the address before the deferred bind).One deliberate divergence: after Node's implicit bind, a subsequent
socket.bind(port)orsocket.send(...)fails withEINVAL, because libuv binds the fd while the JS-level bind state stays unbound, and the later JS-level bind retries thebind(2)syscall on the already-bound fd. Bun marks the socket bound instead, so a laterbind()throwsERR_SOCKET_ALREADY_BOUNDsynchronously, exactly as Node does (#33037, which landed onmainduring review, changed that from an"error"emit to a throw), and a latersend()just works. Reproducing theEINVALwould mean deliberately creating a second, conflicting native socket.A second bug this surfaced:
dns.lookupinvokes its callback twiceReview on this PR pointed out that emitting
listeningsynchronously from the lookup callback means a throwinglisteninglistener would re-enter that callback. Investigating it turned up a pre-existingnode:dnsbug (reproducible onmain, independent of this PR):lookup()invokes the user callback from inside the.thenof a.then(...).catch(...)chain, so the callback's own throw falls into the chained.catch, which invokes the callback a second time with that throw presented as the lookup error. Node invokes the callback exactly once and surfaces the throw asuncaughtException. Fornode:dgram, that double invocation would have turned a throwinglisteninglistener intobindState = UNBOUNDwith a livehandle.socket, so the nextsend()would silently create and leak a second socket.The fix leaves the callback invocation exactly where it is (synchronous, inside the same
.thenreaction) and wraps only that invocation in atry/catchthat re-raises the callback's own throw from aqueueMicrotask. It surfaces as anuncaughtExceptionexactly as in Node, and it can never reach the chained.catch. The error arm gets the same treatment.throwIfEmpty's intentional throw into the.catchis unaffected, and the wholenode:dnsmodule keeps invoking every callback exactly once.An earlier iteration of this fix deferred the callback through
process.nextTickinstead. That regressed the upstreamtest/js/node/test/parallel/test-net-connect-memleak.js: the test is conservative-GC sensitive (its outcome already flips between a debug and a release build of the samemaincommit),net.connect(port)resolveslocalhostthroughdns.lookup, and adding any deferral (aprocess.nextTickor an extra promise reaction) to that path keeps theonce('connect')listener's closure reachable across the test'sgc(). That reproduces 30/30 on an otherwise unmodifiedmainbuild with no build of this branch needed, which is how it was caught and why the deferral was dropped. The landed shape adds zero deferral and zero allocation to the success path, and the test is 40/40 green against this branch.Verification
New tests, all failing on an unfixed build:
test/js/bun/udp/dgram.test.ts, "membership on an unbound socket": the implicit bind for all four operations plus the udp6 wildcard, that the implicitly bound socket can send, and the preserved not-running/EINVAL behaviors. Four fail withERR_SOCKET_DGRAM_NOT_RUNNING.test/js/bun/udp/dgram.test.ts: a throwinglisteninglistener onbind(0, "localhost", cb)followed by asend()keeps the same socket and port, and the throw surfaces throughuncaughtExceptionlike Node. Fails onmain.test/js/node/dns/dns-lookup-keepalive.test.ts:dns.lookupinvokes its callback exactly once when the callback throws. Fails onmainwith two invocations.test/js/bun/udp/,test/js/node/net/,test/js/node/dns/,test/js/node/test/parallel/test-net-connect-memleak.js(40/40), and the upstreamtest/js/node/test/parallel/test-dgram-*,test-dns*,test-net-connect*, andtest-net-autoselectfamily*suites all pass against the debug build with no failures beyond the pre-existing ones already present with the released binary.Rebase
Rebased onto
mainafter #33037 (dgram.Socket#bind()on an already-bound socket now throwsERR_SOCKET_ALREADY_BOUNDsynchronously instead of emitting"error") and #33035 (abun_core::stringsmodule-path cleanup) landed. The only conflict was the two dgram PRs appending independentdescribeblocks to the same point intest/js/bun/udp/dgram.test.ts; both blocks are kept. #33037 is compatible with and strengthens this change, and its two newbind()tests pass alongside this PR's on the rebased tree (229 passing acrosstest/js/bun/udp/plus the dns test).