Conversation
… arm The half-open arm armed the writable poll and then dispatched on_end. An owner that closes the socket in on_end (uWS HTTP on an idle connection, node:net with allowHalfOpen false) paid one poll change for a socket it closed right after. Dispatch first, then change the poll only when the socket is still open: no events for a socket that on_end shut down, writable otherwise.
|
Status: groundwork for #41785, no behaviour change. How the numbers were measured: release builds of main a7c73fd and of this branch, a
|
|
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 configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughFor half-open sockets that receive EOF, the loop now dispatches ChangesHalf-open EOF Polling
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change preserves the expected reply behavior after a client closes its sending side, with no identified issue requiring a fix before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I traced the final poll state for each thing on_end can do (nothing, us_socket_shutdown, us_socket_pause, a partial us_socket_write, adoption via us_socket_adopt, close) against the old WRITABLE-then-on_end order — the resulting events match in every case (shutdown ends at 0 both ways since us_internal_socket_raw_shutdown masks with READABLE; write and resume already gate READABLE on read_eof, which is set before dispatch), and us_poll_change on epoll, kqueue and libuv dedups equal events so nothing extra is issued. The error+read_fin case skipping the poll change is fine because line 956 raw-closes right after. Given this is the shared dispatch path for every half-open socket, a human look is still worthwhile.
Extended reasoning...
The change reorders on_end dispatch and the epoll/kqueue/libuv poll change in the half-open EOF arm of us_internal_dispatch_ready_poll in packages/bun-usockets/src/loop.c, gating the poll change on the socket surviving on_end and selecting 0 events for a shut-down socket. It touches no security-sensitive surface but is the shared event-loop path for Bun.serve, node:net/tls/http2 and allowHalfOpen sockets on all backends. The end poll state was checked to be identical to main for each on_end outcome and the returned handle is validated via us_internal_socket_follow_adopted and us_socket_is_closed before use, but the PR ships no automated test and the code is core native dispatch, so a human review is warranted rather than approval.
…er the client's FIN
There was a problem hiding this comment.
I reviewed the updated PR (including the new regression test in 889ba59) and found no bugs; since this reorders user-callback dispatch against the poll change in usockets' core EOF arm, a human look is still worthwhile.
What was reviewed:
- Traced the three on_end outcomes at packages/bun-usockets/src/loop.c:936-940 against the old order: close (no poll change, error close still runs at line 956), shutdown (us_internal_socket_raw_shutdown masks to READABLE, then the new change sets 0 — same end state as before), keep-open (WRITABLE, as before).
- Checked writes issued from on_end: us_internal_rearm_writable already omits READABLE when read_eof is set, so the final poll change is a no-op there; deferred TLS shutdown (ssl_shutdown_after_spill) leaves the poll type as SOCKET, so it still arms WRITABLE.
- us_dispatch_end always returns the socket (uws_sys vtable trampoline), so the NULL-skip cannot leave the readable poll armed on an open socket.
- The new test wires error/close on both sides to the awaited promises and asserts exact byte count with content comparison; it passes on main too (no behavior change claimed), so it guards the arm rather than proving the reorder.
Extended reasoning...
The change reorders on_end dispatch before us_poll_change in the allowHalfOpen EOF arm of us_internal_dispatch_ready_poll in packages/bun-usockets/src/loop.c, making the poll change conditional on the socket surviving on_end, not being error-flagged, and selecting 0 events when on_end shut the socket down; a describe.each test with three cases is added to test/js/node/net/node-net-allowHalfOpen.test.js. It touches no security-sensitive surface. The diff is small and I traced the resulting poll state as equivalent to the old order for close, shutdown, keep-open, write-from-on_end and deferred TLS shutdown paths, but this is the core socket event loop shared by epoll, kqueue and libuv and the author did not run kqueue or libuv locally, which is why a human look is preferred over approval. The prior run's inline nit asked for a test; the second commit added one, so there is no outstanding objection.
|
@robobun wake up!! |
|
@robobun wake up!! |
|
Here. Status of the stack:
|
This branch has not been deployed
2 participants
Problem
us_internal_dispatch_ready_poll(packages/bun-usockets/src/loop.c), the loop changes the poll to writable and then dispatcheson_end. An owner that closes the socket there pays for a poll change it does not need.Bun.servemakes 1epoll_ctl(MOD), anode:netserver makes 2.Fix
on_endfirst. Then change the poll only when the socket is still open and no error close follows: no events for a socket thaton_endshut down, writable otherwise.node-net-allowHalfOpen.test.js, and the existing suites in Notes.Behaviour change: none
Background
allow_half_openstays open after the peer's FIN. The loop stops the readable poll, keeps the writable poll, and the owner decides when to close.Bun.serve,node:http), the native handles ofnode:net,node:tlsandnode:http2, andBun.listen/Bun.connectwithallowHalfOpen: true.Downsides
us_internal_dispatch_ready_pollgrows by 52 B and 15 instructions (clang -O3, no LTO). The release binary is 80827976 B before and after.Notes
epoll_ctl(MOD)is 0.00 per connection before and after.node:netserver sends an 8 MiB reply to a client that sent its request and its FIN at once, forallowHalfOpentrue and false and for a reply from the'data'or the'end'listener. They pass on main too: the PR changes one syscall, not behaviour.LD_PRELOADshim that counts libc calls (no strace on the machine). Per connection = (count at N=300 - count at N=100) / 200, two runs each, identical. The peer is a separate node process that reads the reply and then sends its FIN. Other calls do not change:epoll_ctlADD 1 and DEL 1,send1,recv2,TCP_CORK0.node:netdrops 2 poll changes, not 1: on main the loop arms writable, then theshutdown()thaton_endtriggers drops it again.us_internal_socket_follow_adoptedruns before the poll change becauseon_endcan adopt the socket into another group.test/js/bun/net/{socket,tcp-server,socket-syscall-fault}.test.ts,test/js/node/net/{node-net-allowHalfOpen.test.js,node-net.test.ts,node-net-server.test.ts},test/js/node/tls/{node-tls-socket-allow-half-open-option,node-tls-raw-end,node-tls-server,node-tls-connect,tls-syscall-fault}.test.ts,test/js/node/http/{node-http-server-socket-end-drain,node-http-server-close-drain,node-http-backpressure,node-http-req-socket-pause,node-http}.test.ts,test/js/node/http2/node-http2.test.js,test/js/bun/http/{serve,node-http-halfclose-midupload}.test.ts, the ninetest/js/bun/test/parallel/test-net-half-open-peer-reset-*.mjs, andtest/js/node/test/parallel/{test-net-allow-half-open,test-net-socket-no-halfopen-enforcer,test-tls-socket-allow-half-open-option}.js.socket.test.ts1 (should not call drain before handshake, needs the public internet),node-net.test.ts11 (10 fromlocalhostresolution in the container, 1 leak test that reaches its 60 s limit on debug builds),serve.test.ts2 (/bun:infoloopback check and the root-range port test, the container runs as root).