-
Notifications
You must be signed in to change notification settings - Fork 5k
usockets(win): defer READABLE re-arm past on_open so .end() mid-handshake doesn't hang #30028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2dc4f5c
212d7e8
4782e95
1c3cbf9
4c4f36f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,8 +21,8 @@ | |
|
|
||
| #ifdef LIBUS_USE_LIBUV | ||
|
|
||
| /* uv_poll_t->data always (except for most times after calling us_poll_stop) | ||
| * points to the us_poll_t */ | ||
| /* uv_poll_t->data points to the owning us_poll_t for the handle's whole | ||
| * lifetime (set in us_create_poll, cleared only by close_cb_free_poll). */ | ||
| static void poll_cb(uv_poll_t *p, int status, int events) { | ||
| us_internal_dispatch_ready_poll((struct us_poll_t *)p->data, status < 0 && status != UV_EOF, status == UV_EOF, | ||
| events); | ||
|
|
@@ -44,8 +44,10 @@ | |
|
|
||
| /* This one is different for polls, since we need two frees here */ | ||
| static void close_cb_free_poll(uv_handle_t *h) { | ||
| /* It is only in case we called us_poll_stop then quickly us_poll_free that we | ||
| * enter this. Most of the time, actual freeing is done by us_poll_free. */ | ||
| /* h->data is the owning us_poll_t (the embedding us_socket_t allocation). | ||
| * us_poll_free arms this; freeing here (after uv_close completes) is the | ||
| * only path — the synchronous-free branch was removed when uv_close moved | ||
| * out of us_poll_stop. */ | ||
| if (h->data) { | ||
| free(h->data); | ||
| free(h); | ||
|
|
@@ -76,18 +78,23 @@ | |
| free(p); | ||
| return; | ||
| } | ||
| /* The idea here is like so; in us_poll_stop we call uv_close after setting | ||
| * data of uv-poll to 0. This means that in close_cb_free we call free on 0 | ||
| * with does nothing, since us_poll_stop should not really free the poll. | ||
| * HOWEVER, if we then call us_poll_free while still closing the uv-poll, we | ||
| * simply change back the data to point to our structure so that we actually | ||
| * do free it like we should. */ | ||
| /* uv_close lives here (not in us_poll_stop) so it runs from check_cb via | ||
| * us_internal_free_closed_sockets — i.e. *outside* poll_cb. us_poll_stop is | ||
| * reachable re-entrantly inside the handle's own poll_cb (connect-WRITABLE → | ||
| * us_internal_socket_after_open → us_poll_change/uv_poll_start(READABLE) → | ||
| * on_open → JS end() → us_internal_socket_close_raw → us_poll_stop), and | ||
| * uv_close-ing a uv_poll_t whose AFD poll request was submitted in the same | ||
| * frame leaves the handle wedged (close_cb never fires; observed as a | ||
| * full-process hang on Windows for `Bun.connect` TLS sockets ended from | ||
| * `open`). Deferring to here keeps every us_poll_stop caller's semantics | ||
| * (they all park on closed_head/closed_udp_head and reach this), just one | ||
| * loop tick later. */ | ||
| p->uv_p->data = p; | ||
| if (uv_is_closing((uv_handle_t *)p->uv_p)) { | ||
| p->uv_p->data = p; | ||
| } else { | ||
| free(p->uv_p); | ||
| free(p); | ||
| /* Shouldn't happen anymore, but tolerate a caller that already closed. */ | ||
| return; | ||
| } | ||
| uv_close((uv_handle_t *)p->uv_p, close_cb_free_poll); | ||
| } | ||
|
|
||
| void us_poll_start(struct us_poll_t *p, struct us_loop_t *loop, int events) { | ||
|
|
@@ -115,17 +122,12 @@ | |
| } | ||
| } | ||
|
|
||
| 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. */ | ||
| } | ||
|
Check failure on line 130 in packages/bun-usockets/src/eventing/libuv.c
|
||
|
Comment on lines
125
to
130
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Windows CI is red on 1c3cbf9: 7 Extended reasoning...What the bug isrobobun's CI report for commit The code pathMoving
Pre-PR, Why existing code doesn't prevent it
Step-by-step proof
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. ImpactThis regresses the common close path — every How to fixPreserve the pre-PR ordering —
Either way, this needs to be root-caused against the failing tests on a Windows box before merge. |
||
|
|
||
| int us_poll_events(struct us_poll_t *p) { | ||
| return ((p->poll_type & POLL_TYPE_POLLING_IN) ? LIBUS_SOCKET_READABLE : 0) | | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.