Repository navigation
Conversation
The Linux error pass of the UDP case in us_internal_dispatch_ready_poll called the error handler between two recvmsg(MSG_ERRQUEUE). A handler that sends again to a closed port on the same host queues the next report before send() returns, so the pass never found the queue empty. The pass now takes every report off the queue first, counted by errno, and then calls the handler once for each report. After the handlers it reads SO_ERROR once and reports that errno when no queued report carries it.
|
Status: ready for review. How to reproduce: run the script of #44218 on Linux.
The test CI: build 122940 (the first commit) is green. Build 123162 (the current head, which adds a test fix and restores two comments) has one failed job: |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughLinux UDP polling now collects queued error reports before invoking callbacks, then checks for a pending socket error. Linux-only helpers expose pending and queued error state. New tests cover repeated errors, datagram delivery, and event-loop ordering. ChangesLinux UDP error handling
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The reviewed change has no identified merge-blocking issue. The added Linux UDP tests still need to complete in CI. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Beyond the inline findings, I also checked two things: the error queue is still drained when on_recv_error is NULL (the take runs unconditionally and recv_error_surfaced keeps its old meaning, so the quic no-op-callback case is unchanged), and the new code is entirely __linux__-gated with no sibling kqueue/Windows UDP path that uses IP_RECVERR, so there is no parallel backend left unfixed.
Extended reasoning...
The change reworks the Linux-only EPOLLERR branch of the UDP poll dispatch in packages/bun-usockets/src/loop.c into a two-phase drain-then-dispatch, adds two getsockopt/poll helpers in bsd.c, updates comments in internal.h, udp_socket.rs and dgram.ts, and adds ~495 lines of Linux-only tests. It touches no auth, crypto or input-parsing surface; the one kernel-reported length (cmsg_len) gains a bounds check. Three confirmed findings (test race, errno reordering within one event, gVisor double-report) are posted inline, so a human look is already signaled; this note only records the extra items ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
refusedSends() detected a late ICMP error only when the second send of a pair did not fail. The pending error of that late ICMP error then failed the first send of the next pair, and the exception failed the test. Every send that a late ICMP error can fail now counts as late, so the scenario is set up again.
|
Updated 11:34 PM PT - Oct 2nd, 2026
❌ @robobun, your commit d46efc7 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 44461That installs a local version of the PR into your bun-44461 --bun |
There was a problem hiding this comment.
I re-reviewed after the test-only push and found no bugs; because the usockets UDP error-dispatch change in loop.c is unchanged since my last pass and alters when and in what order Linux ICMP reports reach handlers, a human look there is still worthwhile.
What was reviewed:
- The new
refused()helper and its use inrefusedSendsand the in-handlersend("retry")sites (test/js/bun/udp/udp_socket_recv_flags.test.ts:212-237, :532, :575): a late-ICMPECONNREFUSEDon any of those sends now counts aslateand re-runs the scenario viafirstUsable, which closes the uncaught-throw path I raised earlier. sendTooLargenow returns a late count (:275-285), so theotherErrnomatrix feedsrefusedSendsthe same way.- loop.c, bsd.c, bsd.h, internal.h: no changes since the prior review; the drain-then-dispatch loop, the
!u->closedguard between handler calls, and the SO_ERROR/poll pair were re-read with nothing new found.
Extended reasoning...
The diff reworks the Linux-only MSG_ERRQUEUE drain in packages/bun-usockets/src/loop.c for us_udp_socket_t (take all reports into a per-errno tally, then dispatch, then read SO_ERROR and poll for POLLERR), adds two getsockopt/poll helpers in bsd.c/bsd.h, updates a comment in internal.h, dgram.ts and udp_socket.rs, and adds ~500 lines of Linux-gated tests. It touches no auth, crypto, or injection surface; the sensitive part is event-loop dispatch order and syscall-count per UDP error event. The only change since the prior review is the test commit 985e4ae, which addresses my earlier inline finding at the first send of refusedSends. The native change is non-trivial and changes observable errno ordering and per-event syscalls, so it is not a simple/mechanical change suitable for approve without a human.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
internal.h holds the contract of is_errqueue. The two callers keep the wording of main.
Fixes #44218
Problem
errorhandler that sends again to a closed port on the same host never leaves one poll dispatch:100000 'error' events; setImmediate ran 0 times; the 1 ms timer fired: false.us_internal_dispatch_ready_poll(packages/bun-usockets/src/loop.c:1004) calls the handler between tworecvmsg(MSG_ERRQUEUE). Over loopback the handler's datagram queues its report beforesend()returns.Fix
SO_ERRORonce: the pending errno of a handler's datagram would fail the nextrecvmmsg. The pass reports it only if no report is queued.test/js/bun/udp/udp_socket_recv_flags.test.ts(15 new tests, 6 fail on main). Also the UDP,dgram, QUIC and HTTP/3 suites.Background
IP_RECVERRmakes the kernel queue one report for each ICMP error and raise EPOLLERR while one is queued. Bun sets it on each UDP socket with an error callback.SO_ERRORread, butmessagearrives beforeerror.Downsides
getsockopt), 2 after a handler sent. Release text: +512 bytes.Notes
Who can hit it. All of these must hold:
send()returns.Bun.udpSocket, or anode:dgramsocket that is connected.dgram.tsdrops the reports of a socket that is not connected.errorhandler sends again before the loop runs again: in the handler, inprocess.nextTick, or after anawait. A send fromsetImmediatedoes not hang on main.No user reported it. A review of the UDP dispatch found it. The drain loop is in releases since v1.3.14 (#29768).
Why the kernel does not pace this loop. A maintainer said of the TCP read loop (#44220) that the kernel paces the reads. That loop needs a peer to make its input. This pass does not: the
send()of the handler queues the next report before it returns, so each pass of the loop made the input of the next pass.The class, and what this PR leaves.
us_internal_dispatch_ready_pollhas four loops of this shape: this error pass, the UDP data loop, the accept loop and the TCP read loop. A handler can feed the first three with no peer. This PR changes the error pass only. With this PR the other two still keep the loop in one event:datahandler that sends a datagram to its own socket gets 2000 calls in 1 loop iteration. usockets(udp): bound a readable event at 32 datagrams #37103 is the PR for the data loop. 8133dd1 reverted an earlier cap there (Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075).openhandler that connects to its own listener gets 1000 connections in 1 loop iteration. usockets: bound the accepts of one listener readiness event #44198 is the PR for the accept loop.Why the error pass is a unit of its own. Only the error queue has these two properties:
The rule. One event handles what its queue held when the event began. What a handler queues belongs to the next event, after the timers and the other polls. This PR applies the rule to the error queue, where it is exact. The data loop and the accept loop cannot take their queue before the handler runs, so #37103 and #44198 use a count of 32. If a maintainer wants the same rule there, those counts become a bound from a capacity (the receive buffer, the listen backlog).
The 221 calls. A handler that sends 3 datagrams queues more than 1 report for the next event (about half of its sends fail with the pending error). So the number of reports grows from event to event until the receive buffer is full (221 reports). Node gets 1 error for each loop turn because it never sets
IP_RECVERR: the kernel then keeps one pending error.node:dgramwithoutIP_RECVERRis the change that ends this. It is not built yet, and it is not in this PR.The
SO_ERRORread. It is not part of the fix for the hang. It keeps two behaviours of main. On main the loop took the report of the handler's datagram in the same event, and the kernel clears the pending error when the last report leaves the queue.pollruns only ifSO_ERRORwas not zero, that is after a handler sent a datagram that was refused. If no report is queued then, the kernel had no room for one, and the pass reports the errno once witherrqueuenot set. Main reported that errno from therecvmmsgthat failed with it.Measurements. Release builds of main (f4d755a) and of this PR on that commit, Linux x64, kernel 7.0.0. An
LD_PRELOADlibrary counted the calls.gdbsingle steps counted the instructions thatus_internal_dispatch_ready_pollitself executes.recvmmsg, 0 error-queue callsrecvmsgrecvmsg, 1getsockoptrecvmsgrecvmsg, 100getsockopt, 99pollrecvmsgrecvmsg, 1getsockoptrecvmsggetsockoptrecvmsg, 3recvmmsgrecvmsg, 2recvmmsg, 1getsockopt, 1pollECONNREFUSEDThe row with 8 reports is a change in behaviour. At each dequeue the kernel sets the pending error to the errno of the next queued report, so on main a send from the handler fails while reports wait. With this PR the queue is empty when the handlers run, and only the send after a refused datagram fails.
What stays as on main.
send()still report the pending error while its report is queued (udp: stop IP_RECVERR on unconnected sockets; never fatal-by-default for recv errors #35922 describes it).node:quicendpoint has an error callback that does nothing, soudp.c:238gives itIP_RECVERR. The pass takes its reports and drops them.pkg/tcpip/transport/udp/endpoint.go,ReadinessandonICMPError). So main reports each ICMP error two times there, in two events. This PR reports it two times in one event. The review asked for one more read of the error queue for that case. It is not here: the number of reports is the same as on main, and no gVisor run exists to test it.Tests. The block
error queue (IP_RECVERR)has 15 new tests. They count theerrorcalls bygetEventLoopStats().iteration.an error handler that sends again gets the next report in the next loop iterationfail: 100 errors in one iteration, and the immediate did not run.SO_ERRORread.SO_ERRORread, the read never reports, the read without thepoll, the read only without a datagram, the read only with a datagram, a handler call after the close, one report for each event, one errno slot, noIPV6_RECVERRbranch.udp_socket_recv_flags(17),udp_socket(230),dgram(62), node'stest-dgram-*(78 files),test-quic-endpoint-*andtest-quic-sni*(15 files),quic-endpoint(7),quic-stream(7),quic-sni(6),fetch-http3-client(65),serve-http3(73).Other open PRs in this code.
test/_util/loop-iterations.ts. The PR that lands second must keep one copy of the helpers that count by loop iteration.IP_RECVERRat connect time only. The tests here use connected sockets, so they hold after it.errorhandler ofBun.udpSocketwith(socket, error). The tests here read the error as the last argument, so they hold after it.recvmsgandsendmsgfallback for a seccomp filter that refusesrecvmmsg. The pass usesrecvmsg,getsockoptandpoll, so it needs no fallback.src/usockets/udp.rshas the same drain loop and needs the same change.This PR, #35922 and #37103 change the same switch case. They need one decision: does
Bun.udpSocketkeepIP_RECVERRon every socket, and doesnode:dgramdrop it as node does.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/udp/udp_socket_recv_flags.test.ts