Conversation
…onously on bind failure
Bun.udpSocket() is declared as returning Promise<UDPSocket>, but when the
underlying bind failed (EADDRINUSE, EADDRNOTAVAIL, unresolvable hostname) or
option validation rejected the input, the native host function threw
synchronously before a promise was ever returned. That meant the common
resilience patterns
Bun.udpSocket({ port }).catch(() => Bun.udpSocket({ port: 0 }))
Promise.allSettled([...portRange.map(p => Bun.udpSocket({ port: p }))])
never had a chance to observe the error: the synchronous throw escaped the
promise chain entirely.
Route the host function through a small wrapper that takes any pending
exception off the VM and returns it as a rejected promise (same shape as
fetch()'s reject_on_exception). Termination exceptions still propagate
synchronously. The error-path scopeguard in the implementation body is
unchanged, so the failure-time wrapper cleanup (Strong-ref downgrade and
socket close) still runs before the rejected promise is created.
WalkthroughChangesUDP socket Promise errors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/runtime/socket/udp_socket.rs`:
- Line 570: Update the OutOfMemory match arm in the UDP socket error handling to
return Err(bun_jsc::JsError::OutOfMemory) directly, matching the
termination/error propagation path. Remove the
global_this.create_out_of_memory_error() conversion while preserving all other
error branches unchanged.
In `@test/js/bun/udp/udp_socket.test.ts`:
- Around line 92-97: Update the test.each case for invalid ports to call
udpSocket directly without wrapping it in Promise.resolve().then(); first assert
the call returns a Promise, then assert that Promise rejects with the expected
error message, preserving coverage for synchronous-throw behavior.
- Around line 167-169: Update the rejection assertions in the EADDRNOTAVAIL test
to also verify that rejection.code equals "EADDRNOTAVAIL", while preserving the
existing non-null and bind syscall checks.
- Around line 138-143: Ensure both fulfilled UDP sockets are always closed even
when assertions fail: in test/js/bun/udp/udp_socket.test.ts:138-143, wrap the
fallback socket assertions in try/finally with socket.close(); in
test/js/bun/udp/udp_socket.test.ts:195-202, wrap the assertions for
results[1].value in try/finally and close that socket in the finally block.
🪄 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: 392f03ec-77c3-4d76-aace-138d94ba8570
📒 Files selected for processing (2)
src/runtime/socket/udp_socket.rstest/js/bun/udp/udp_socket.test.ts
|
Updated 2:23 AM PT - Jul 23rd, 2026
❌ @robobun, your commit 011b38b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35217That installs a local version of the PR into your bun-35217 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped fix that brings Bun.udpSocket() in line with its Promise<UDPSocket> type signature.
Checked: the wrapper propagates Terminated/OutOfMemory unchanged and only converts Thrown to a rejected promise; try_take_exception clears the VM exception before Ok(...) returns; the error-path scopeguard in the renamed udp_socket_impl is untouched so the Strong-ref downgrade and socket close still run. node:dgram's startBunSocket handles both shapes identically (outer try/catch and .$then(_, err) route to the same emit('error')), so the only observable difference there is sync→microtask timing, which is closer to Node anyway. New tests call udpSocket() directly, assert instanceof Promise before awaiting rejection, and close every fulfilled socket in finally. The one deferred item (sharing the exception-to-rejection adapter with fetch.rs) is reasonable to leave for a follow-up.
Extended reasoning...
Overview
Two files: src/runtime/socket/udp_socket.rs gains a ~15-line wrapper (udp_socket) around the existing implementation (renamed udp_socket_impl, otherwise byte-identical) that converts a pending JS exception into a rejected promise; test/js/bun/udp/udp_socket.test.ts adds a describe block with five tests covering EADDRINUSE, EADDRNOTAVAIL, invalid options, .catch() fallback, and Promise.allSettled, plus tightens the existing out-of-range-port test.each to assert a Promise is returned.
Security risks
None. No new syscalls, no untrusted-input parsing changes, no auth/crypto. The change only alters how an already-constructed error value is surfaced to JS (thrown vs. rejected).
Level of scrutiny
Low-to-moderate. This is a user-facing behavior change on a Bun-native API, but it aligns the implementation with the documented Promise<UDPSocket> return type, and code using await (the normal shape) is unaffected because await re-throws rejections. The implementation follows the established reject_on_exception pattern from fetch.rs — the shape is not novel. I verified try_take_exception clears the VM exception (so returning Ok afterward is safe for to_js_host_call), the termination-recheck guards against a termination surfaced as Thrown, and OOM propagates via Err per the CodeRabbit thread. The scopeguard at the top of udp_socket_impl is unchanged, so the existing leak test ("does not leak UDPSocket wrapper when creation fails") still exercises the Strong-ref downgrade on the error path — that test now goes through await + rejection, which is equivalent.
Other factors
All four CodeRabbit findings and my two prior inline nits are addressed in 0b4e68e: OOM now propagates, the test.each calls udpSocket() directly and asserts instanceof Promise, the EADDRNOTAVAIL test asserts code === 'EADDRNOTAVAIL', and both the .catch() and allSettled tests wrap their fulfilled sockets in try/finally (the allSettled finally iterates all results). The author's rationale for keeping the adapter local rather than lifting into bun_jsc (intentional divergence on OOM handling and promise constructor; cross-crate refactor out of scope for a focused bugfix) is sound. node:dgram was checked at src/js/node/dgram.ts:703-778 — both the sync catch and the .$then rejection handler set bindState = BIND_STATE_UNBOUND and emit 'error', so no compat regression. The one platform-sensitive assertion is EADDRNOTAVAIL for binding to 192.0.2.1 — RFC 5737 TEST-NET-1, which should reliably produce that errno on all CI platforms; if a lane disagrees CI will surface it.
There was a problem hiding this comment.
No bugs found; all prior feedback (OOM propagation, test cleanup in finally, direct-call-not-wrapped assertions, EADDRNOTAVAIL code check) is addressed in the current diff, and the shared-helper extraction is reasonably deferred as a follow-up. Deferring to a human only because this is a user-observable behavior change to a Bun-native API — config-validation and bind failures now reject instead of throwing synchronously — which is worth a maintainer sign-off.
What was reviewed:
- The error-path
scopeguardinudp_socket_implstill runs (Strong-ref downgrade + socket close) before the wrapper takes the exception, so the existing leak test remains valid. node:dgram'sstartBunSocketwraps the call in bothtry/catchand.then(ok, err), so it handles either shape unchanged.TerminatedandOutOfMemorypropagate asErrrather than being converted to rejections;rejected_promisematches the constructor used elsewhere (streams.rs).- New tests close every fulfilled socket in
finally, calludpSocket()directly and assertinstanceof Promisebefore awaiting the rejection.
Extended reasoning...
Overview
The PR wraps UDPSocket::udp_socket in a ~15-line adapter that renames the original body to udp_socket_impl and, on Err(JsError::Thrown), takes the pending exception off the VM and returns it as JSPromise::rejected_promise(...). Terminated and OutOfMemory still propagate as Err. The test file gains a describe block with five cases (EADDRINUSE, EADDRINUSE-via-.catch, EADDRNOTAVAIL, invalid options, Promise.allSettled) plus the existing connect.port range test is updated to assert instanceof Promise + rejects.toThrow instead of a bare sync toThrow.
Security risks
None. No new input parsing, no allocation-size arithmetic, no untrusted data. The change moves an already-constructed error value from the throw slot into a rejected promise.
Level of scrutiny
Medium. The Rust change is small and follows the reject_on_exception shape in src/runtime/webcore/fetch.rs; I verified the error-path cleanup guard in udp_socket_impl runs before the wrapper observes Err, so the Strong-ref downgrade and socket close still fire and the existing wrapper-leak test still exercises them (it uses try { await udpSocket(...) } catch {}, which catches rejections identically). node:dgram is unaffected because startBunSocket already handled both sync throw and promise rejection. The user-facing observable change is that code doing try { const p = Bun.udpSocket(bad) } catch {} without await will no longer catch — but that pattern contradicts the documented Promise<UDPSocket> return type, so this is a correctness fix.
Other factors
All four CodeRabbit findings and both of my earlier inline comments are resolved in the current diff (0b4e68e): OOM now returns Err directly, the invalid-port test calls udpSocket() unwrapped and asserts a Promise is returned, the EADDRNOTAVAIL test asserts the specific code, and the fallback/allSettled tests close fulfilled sockets in finally. The one remaining suggestion — lifting the exception-to-rejection adapter into a shared bun_jsc helper — was declined for this PR as a cross-crate refactor riding on a focused bugfix, which is a defensible read of REVIEW.md.
I'm deferring rather than approving because this changes the user-observable error surface of Bun.udpSocket() (validation errors like port: -1 now reject instead of throwing synchronously). It's the right change given the Promise return type and the fetch precedent, but Bun-native API-surface behavior changes are worth a maintainer glance.
|
CI status: the diff is green. The two remaining red lanes on 78420 are unrelated to this change:
Neither touches UDP, dgram, or anything on the promise-rejection path this PR changes. Ready for review. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-23, 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. |
2 participants
What
Bun.udpSocket()is typed asPromise<UDPSocket>, but when the underlying bind failed (EADDRINUSE, EADDRNOTAVAIL, unresolvable hostname, invalid options) the native host function threw synchronously before a promise was ever returned. That meant.catch()/.then(ok, err)/Promise.allSettlednever saw the error:so the fallback-port pattern
Bun.udpSocket({port}).catch(() => Bun.udpSocket({port: 0}))never applied.Cause
UDPSocket::udp_socketreturnedErr(global_this.throw_value(...))on every failure path (config validation,us_create_udp_socketreturning null, connect failure), which surfaces to JS as a synchronous throw viato_js_host_call.Fix
Route the body through a wrapper that takes any pending exception off the VM and returns it as a rejected promise, matching the
reject_on_exceptionshape used byfetch()insrc/runtime/webcore/fetch.rs. Termination and OOM still propagate. The error-pathscopeguardin the implementation body is unchanged, so the Strong-ref downgrade and socket close still run before the rejected promise is created (the existing leak test covers this).node:dgramis unaffected:startBunSocketalready wrapped the call in bothtry/catchand.then(ok, err), so it handles either shape.Not addressed here: the
ENOENTcode for an unresolvable bind hostname. That comes frombsd_create_udp_socketwriting*err = -gai_result, which on glibc collapsesEAI_NONAME(-2) into errno 2 (ENOENT). Fixing it cleanly means threading a separate getaddrinfo error channel throughus_create_udp_socket, which is better done in its own change.Verification
New tests under
returns a rejected promise on failure instead of throwing synchronouslyfail on current main and pass with this change.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/udp/udp_socket.test.ts