usockets(win): defer READABLE re-arm past on_open so .end() mid-handshake doesn't hang - #30028
usockets(win): defer READABLE re-arm past on_open so .end() mid-handshake doesn't hang#30028Jarred-Sumner wants to merge 5 commits into
Conversation
…andshake doesn't hang
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a TLS socket regression test that exercises calling Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@packages/bun-usockets/src/context.c`:
- Around line 776-779: The post-open re-arm always sets LIBUS_SOCKET_READABLE
which can re-enable reads if user code paused the socket in open; modify the
mask passed to us_poll_change so it includes LIBUS_SOCKET_READABLE only when
s->flags.is_paused is false (i.e., compute an event_mask that conditionally adds
LIBUS_SOCKET_READABLE based on s->flags.is_paused, still preserving the existing
writable logic that checks s->flags.last_write_failed) before calling
us_poll_change in the block guarded by us_socket_is_closed.
🪄 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: 8b08930c-eee9-402e-9554-6553c8c2f543
📒 Files selected for processing (1)
packages/bun-usockets/src/context.c
Replaces the context.c special-casing. Root cause is uv_close on a uv_poll_t inside its own poll_cb (after us_poll_change just submitted a fresh AFD request). All us_poll_stop callers park on closed_head and reach us_poll_free from check_cb (outside poll_cb), so closing there avoids the re-entrancy without changing socket_after_open semantics on any platform.
| void us_poll_stop(struct us_poll_t *p, struct us_loop_t *loop) { | ||
| if(!p->uv_p) return; | ||
| uv_poll_stop(p->uv_p); | ||
|
|
||
| /* We normally only want to close the poll here, not free it. But if we stop | ||
| * it, then quickly "free" it with us_poll_free, we postpone the actual | ||
| * freeing to close_cb_free_poll whenever it triggers. That's why we set data | ||
| * to null here, so that us_poll_free can reset it if needed */ | ||
| p->uv_p->data = 0; | ||
| uv_close((uv_handle_t *)p->uv_p, close_cb_free_poll); | ||
| /* uv_close deferred to us_poll_free — see comment there for why closing | ||
| * here (inside poll_cb) wedged the handle on Windows. */ | ||
| } |
There was a problem hiding this comment.
🔴 Windows CI is red on 1c3cbf9: 7 node:http parallel tests (test-http-{flush-response-headers,client-timeout-option,client-timeout-event,response-close,client-timeout,agent-remove,client-finished}.js) time out on all three Windows targets — that's a systematic regression in the normal close path, not flake. Deferring uv_close to us_poll_free inverts the order at every close site (e.g. socket.c:289→298): closesocket(fd) now runs before uv_close on the uv_poll_t, whereas pre-PR uv_close ran synchronously inside us_poll_stop before closesocket. This needs root-causing before merge — right now the PR trades one Windows hang for several others.
Extended reasoning...
What the bug is
robobun's CI report for commit 1c3cbf9 (the current libuv.c approach in this PR — 4c4f36f is docs-only) shows 7 distinct node:http parallel tests timing out on all three Windows targets (2019 x64, 2019 x64-baseline, 11 aarch64): test-http-flush-response-headers, test-http-client-timeout-option, test-http-client-timeout-event, test-http-response-close, test-http-client-timeout, test-http-agent-remove, test-http-client-finished. ~20 timeout occurrences clustered in one functional area across every Windows build is not flakiness — it's a deterministic regression in the everyday socket-close path. The lone macOS no-orphans timeout is unrelated noise.
The code path
Moving uv_close from us_poll_stop to us_poll_free flips the relative order of uv_close and closesocket() at every close site:
socket.c:289→us_poll_stop…socket.c:298→bsd_close_socket(fd)… later,check_cb→us_internal_free_closed_sockets→us_poll_free→uv_close.- Same pattern at
socket.c:198→199,context.c:411→412,udp.c:105→106.
Pre-PR, us_poll_stop did uv_poll_stop then uv_close synchronously, so by the time bsd_close_socket/closesocket ran the handle was already UV_CLOSING and libuv's uv__poll_close had cancelled the AFD poll request against a still-live SOCKET. Post-PR, the SOCKET is dead before libuv ever sees the close.
Why existing code doesn't prevent it
uv_poll_stop alone does not cancel the outstanding AFD poll request on Windows — it only clears the event mask and unrefs the handle; the AFD cancellation happens in uv__poll_close (reached via uv_close). Nothing else in this PR re-orders bsd_close_socket after us_poll_free, and the sweep that drains closed_head runs from check_cb strictly after the closesocket() at socket.c:298. So every normal HTTP request that closes its socket now hits closesocket-before-uv_close.
Step-by-step proof
- A node:http test opens a client connection, gets a response, and closes (
socket.destroy()/req.abort()/ agent free). - That reaches
us_socket_close→ socket.c:289us_poll_stop(p). Post-PR this is justuv_poll_stop(p->uv_p)— handle is stopped, not closing; the AFD poll request submitted by the lastuv_poll_startis still pending in the kernel. - socket.c:298
bsd_close_socket(fd)→closesocket(). The underlying SOCKET is gone. - The socket is parked on
closed_head. On the nextcheck_cbtick,us_internal_free_closed_socketscallsus_poll_free→uv_close(p->uv_p, close_cb_free_poll). - libuv's
uv__poll_closetries to cancel the pending AFD request via the (now-dead) socket / peer socket. If that cancellation can't complete,reqs_pendingnever drops to 0,uv__want_endgamenever queues the handle,close_cb_free_pollnever fires, and the handle stays ref'd in the loop — process hangs → test timeout.
Whether the precise mechanism is exactly the AFD-cancel-on-dead-fd path or something adjacent in libuv's Windows poll endgame, the empirical evidence is unambiguous: the PR's stated validation strategy is "Windows CI is the validation here", and Windows CI is red on the exact commit with the exact symptom (hangs in socket close paths). The ordering inversion is the only semantic change at those sites.
Impact
This regresses the common close path — every node:http/fetch connection close on Windows — to fix the rare one (.end() inside open mid-handshake). Shipping this would hang real Windows workloads.
How to fix
Preserve the pre-PR ordering — uv_close must run before closesocket(). Two options:
- Keep
uv_closeinus_poll_stopfor the normal path and only defer whenus_poll_stopis re-entered from inside the same handle'spoll_cb(e.g. a per-loop "currently dispatching poll == p" sentinel that routes the in-frame case onto a deferred-close list drained fromcheck_cb). - Or move
bsd_close_socket(fd)intous_poll_free(afteruv_close) so the fd outlives the AFD cancellation — but that's a larger invariant change touching fd-reuse andSO_LINGERpaths.
Either way, this needs to be root-caused against the failing tests on a Windows box before merge.
|
Heads up: #33018, opened today from an independent investigation, overlaps with this PR. It makes the same change ( Two data points that may save you time, both measured on a Windows debug build:
The full relationship writeup is on #33018. Happy to rebase onto this one if it lands first, or fold the two into one however you prefer. |
|
Two corrections to my comment above, and I think the second one is the reason this PR has been stuck. 1. I wrote that this PR's regression test passes on #33018's branch. That is no longer true. It was true of #33018's first revision, which included the same 2. The The mechanism:
So a socket closed from the timer phase, which runs after the check phase ( The instrumented trace is 14 lines and unambiguous: three For the |
2 participants
.end()on aBun.connectTLS socket from inside itsopenhandler (which fires pre-handshake when ahandshakehandler is present) hung the process on Windows. Surfaced by #30024's new test.Root cause
us_poll_stop(libuv backend) calleduv_closeon theuv_poll_timmediately. Whenopenruns inside the connect-WRITABLEpoll_cb, the sequence isuv_poll_start(READABLE)(fromus_poll_changeatcontext.c:732) → JSopen→end()→us_internal_socket_close_raw→us_poll_stop→uv_close, all in the samepoll_cbframe.uv_closeon a handle whose AFD poll request was just submitted in that frame wedges the handle (close_cb never fires). POSIX has no analogue (epoll_ctl(DEL)/kqueue mask change).Fix
Move
uv_closefromus_poll_stoptous_poll_free. Everyus_poll_stopcaller parks the socket onclosed_head/closed_udp_head;us_internal_free_closed_socketsdrains those fromcheck_cb(auv_check_thandler — runs after I/O callbacks, outsidepoll_cb).uv_closefrom there completes normally on the next tick.us_internal_socket_after_openstays bit-identical to POSIX.Regression test in
socket.test.ts.