usockets(win): defer READABLE re-arm past on_open so .end() mid-handshake doesn't hang - #30028
Open
Jarred-Sumner wants to merge 5 commits into
Open
usockets(win): defer READABLE re-arm past on_open so .end() mid-handshake doesn't hang#30028Jarred-Sumner wants to merge 5 commits into
Jarred-Sumner wants to merge 5 commits into
Claude / Claude Code Review
completed
May 1, 2026 in 15m 22s
Code review found 2 potential issues
Found 6 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | packages/bun-usockets/src/eventing/libuv.c:125-129 |
Stale file-top comment: us_poll_stop no longer zeroes uv_p->data |
| 🟡 Nit | test/js/bun/net/socket.test.ts:835-840 |
Test comment describes the reverted context.c approach, not the shipped libuv.c fix |
Annotations
Check warning on line 129 in packages/bun-usockets/src/eventing/libuv.c
claude / Claude Code Review
Stale file-top comment: us_poll_stop no longer zeroes uv_p->data
Now that `us_poll_stop` no longer does `p->uv_p->data = 0;`, the file-top comment above `poll_cb` ("uv_poll_t->data always (except for most times after calling us_poll_stop) points to the us_poll_t") is stale — the parenthetical exception no longer applies. The comments in `close_cb_free_poll`, `us_poll_free`, and `us_poll_stop` were updated to match the new behavior, but this one was missed; consider dropping the parenthetical.
Check warning on line 840 in test/js/bun/net/socket.test.ts
claude / Claude Code Review
Test comment describes the reverted context.c approach, not the shipped libuv.c fix
Nit: this comment's last sentence ("The fix defers the READABLE re-arm until after on_open returns and skips it if on_open closed the socket") describes the context.c approach from 2dc4f5c/212d7e8/4782e95, which 1c3cbf9 reverted — context.c is no longer in the diff and `us_poll_change(READABLE)` at context.c:732 still runs *before* `on_open` on all platforms. The shipped fix is moving `uv_close` from `us_poll_stop` to `us_poll_free` so it runs from `check_cb` outside `poll_cb`; worth updating th
Loading