-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Bun.serve: close idle connections on graceful stop(), declare closeIdleConnections() #37074
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
Changes from all commits
9839f50
9e7c09d
f680d20
00d3764
4296713
129bce1
9b1f930
4c31af5
e5d6fc8
b3955e9
fb828c2
8b98bd3
a0337dc
7913af6
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 |
|---|---|---|
|
|
@@ -59,7 +59,9 @@ struct HttpResponseData : AsyncSocketData<SSL>, HttpParser { | |
| this->state &= ~HttpResponseData<SSL>::HTTP_RESPONSE_PENDING; | ||
|
|
||
| HttpResponseData<SSL> *httpResponseData = uwsRes->getHttpResponseData(); | ||
| httpResponseData->isIdle = true; | ||
| /* A queued pipelined response (node:http) still owes output on this | ||
| * connection, so it is not idle between the responses. */ | ||
| httpResponseData->isIdle = httpResponseData->nodeHttpQueuedPipelinedCount == 0; | ||
| } | ||
|
|
||
| /* Caller of onWritable. It is possible onWritable calls markDone so we need to borrow it. */ | ||
|
|
@@ -143,13 +145,19 @@ struct HttpResponseData : AsyncSocketData<SSL>, HttpParser { | |
| * into the shared word so the shared response-end path (internalEnd) never | ||
| * has to touch the node-only field. */ | ||
| HTTP_NODE_HAS_RESPONSE_TRAILERS = 1 << 16, | ||
| /* Close this connection the next time it is idle (no request being | ||
| * received, no response in flight or queued). Set by | ||
| * App::closeIdle(true) on connections that were busy during a graceful | ||
| * shutdown sweep; the shouldCloseConnection() gates act on it once the | ||
| * in-flight work completes. */ | ||
| HTTP_CLOSE_WHEN_IDLE = 1 << 17, | ||
|
|
||
| /* Bits that describe the connection rather than the response in flight. | ||
| * There is one HttpResponseData per socket, reused by every request on a | ||
| * keep-alive connection, so starting a new response clears the rest of the | ||
| * word (resetResponseState) - these have to survive that. */ | ||
| HTTP_CONNECTION_SCOPED = HTTP_NODE_PARSING_STOPPED | HTTP_NODE_READS_PAUSED | ||
| | HTTP_NODE_TUNNEL_AFTER_BODY | HTTP_NODE_RECEIVED_FIN, | ||
| | HTTP_NODE_TUNNEL_AFTER_BODY | HTTP_NODE_RECEIVED_FIN | HTTP_CLOSE_WHEN_IDLE, | ||
| }; | ||
|
|
||
| /* Begin a new response on this connection. Clearing the word in one go is | ||
|
|
@@ -158,6 +166,9 @@ struct HttpResponseData : AsyncSocketData<SSL>, HttpParser { | |
| * keep-alive socket; only the connection-scoped bits are carried over. */ | ||
| void resetResponseState() { | ||
| state = (state & HTTP_CONNECTION_SCOPED) | HTTP_RESPONSE_PENDING; | ||
| /* A response is in flight again (a new request dispatched, or a queued | ||
| * pipelined response activated), so the connection is not idle. */ | ||
| this->isIdle = false; | ||
| } | ||
|
|
||
| /* Set or clear a flag from a runtime bool. */ | ||
|
|
@@ -214,7 +225,8 @@ struct HttpResponseData : AsyncSocketData<SSL>, HttpParser { | |
| * any) has completed and all buffered outgoing data has been flushed. */ | ||
| bool shouldCloseConnection() const { | ||
| return (state & HTTP_CONNECTION_CLOSE) | ||
| || ((state & HTTP_NODE_RECEIVED_FIN) && nodeHttpQueuedPipelinedCount == 0); | ||
| || ((state & HTTP_NODE_RECEIVED_FIN) && nodeHttpQueuedPipelinedCount == 0) | ||
| || ((state & HTTP_CLOSE_WHEN_IDLE) && this->isIdle); | ||
|
Comment on lines
225
to
+229
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. 🟡 One layer up from the shims fb828c2/8b98bd35/a0337dc3 guarded: after Extended reasoning...What was left outThree fix commits (fb828c2, 8b98bd3, a0337dc) hardened callers against The code path
Then:
Why nothing else prevents itNone of the shims fb828c2/8b98bd35/a0337dc3 guarded sit between Step-by-step proof
ImpactSame practically-benign class as the three prior accepted-and-fixed findings on this PR: the ext block is inline in a closed-list Pre-existing for Fixbool success = httpResponseData->callOnWritable(...);
/* The onWritable callback may have completed the response and closed the
* socket via a shouldCloseConnection() gate; the ext is destructed. */
if (us_socket_is_closed((us_socket_t *) s)) {
return s;
}right after :737, mirroring the parser's post-dispatch check. This also makes the |
||
| } | ||
|
|
||
| #ifdef UWS_WITH_PROXY | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -479,6 +479,24 @@ impl FileResponseStream { | |
| let resp = self.resp.get(); | ||
| resp.end_send_file(self.sendfile.get().offset, resp.should_close_connection()); | ||
| (self.on_complete.get())(self.ctx.get(), resp); | ||
| // `end_send_file` bypasses every shouldCloseConnection() gate: it does | ||
| // not go through internalEnd, and the onWritable gate is skipped | ||
| // because this frame returns `false` to it. Run the gate here — after | ||
| // `on_complete`, which must see a live socket — so Connection: close | ||
| // and the graceful-stop close-when-idle mark actually close. | ||
| // | ||
| // `resp` is still valid here: usockets never frees a socket | ||
| // synchronously — us_socket_close only links it onto the loop's | ||
| // closed list, freed by us_internal_free_closed_sockets at the end of | ||
| // the loop iteration — so the allocation outlives this frame no | ||
| // matter what `on_complete` did (the same invariant that makes | ||
| // passing `resp` to `on_complete` after the end sound). It is still | ||
| // *this* HTTP socket: an upgrade (us_socket_adopt) is only reachable | ||
| // from a live in-flight request, and this one just completed. And if | ||
| // anything in the frame closed it, the shim's leading | ||
| // us_socket_is_closed check returns before touching the destructed | ||
| // ext. Only `finish()` runs after this, and it never touches `resp`. | ||
| resp.close_if_done_and_marked(); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Collaborator
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. What gurantees resp is still alive and correct here?
Collaborator
Author
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. Three separate guarantees, one per failure mode: Alive (memory): usockets never frees a socket synchronously. us_socket_close only unlinks it and pushes it onto loop->data.closed_head (packages/bun-usockets/src/socket.c:209, 342, 389); the allocation is freed exclusively by us_internal_free_closed_sockets, which the loop runs at the end of the iteration (packages/bun-usockets/src/loop.c:362, called at :449). This whole sequence is one synchronous frame on the loop thread, so the us_socket_t behind resp outlives it regardless of what on_complete did. This is the same invariant end_sendfile already leaned on to pass resp to on_complete after end_send_file, and the one detach_resp's comment documents (uWS may free it on the NEXT loop tick). Correct (still this HTTP socket): the only thing that re-types a socket mid-tick is us_socket_adopt from a WebSocket upgrade, which is only reachable from a live in-flight request dispatch; this request just completed (end_send_file ran markDone), and do_upgrade rejects a responded request. None of the on_complete callees (RequestContext::on_file_stream_complete, FileRoute/DirectoryRoute::on_response_complete) dispatch an upgrade; they do bookkeeping, and server deinit from on_request_complete is deferred via schedule_deinit, never a synchronous app.close(). Closed-but-not-freed (state): if anything in the frame did close the socket (the end itself, or JS reached through a drained microtask calling stop(true)), HttpContext::onClose has destructed the ext block, and the shim handles exactly that: uws_res_close_if_done_and_marked checks us_socket_is_closed first and returns before touching HttpResponseData (src/uws_sys/libuwsockets.cpp). Pushed 7913af6 putting this argument in the comment at the call site so it does not live only in the PR. |
||
| self.finish(); | ||
| } | ||
|
|
||
|
|
@@ -543,6 +561,10 @@ impl FileResponseStream { | |
| let resp = self.resp.get(); | ||
| resp.end_without_body(resp.should_close_connection()); | ||
| (self.on_complete.get())(self.ctx.get(), resp); | ||
| // This end runs uncorked (reader callbacks), so no cork or parser | ||
| // gate will run the close check; do it here, after `on_complete` | ||
| // like `end_sendfile`, so the callbacks see a live socket. | ||
| resp.close_if_done_and_marked(); | ||
|
robobun marked this conversation as resolved.
claude[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // Release the owner ref from `heap::into_raw` in `start()`. Every entry | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.