Don't block HTTP actors on user callbacks - #3059
Conversation
The http.Client, http.Server and the listeners invoked user callbacks in ways that parked the invoking actor until the user's callback activation had completed: the client did "await async on_connect(self)" and "await async cb(self, r)", the listeners awaited on_accept, and the servers called the request handler in value position. A parked actor processes no messages, so a callback that in turn called back into the parked actor and waited for the result would deadlock both actors: the callback actor waits for the parked actor to process its call, while the parked actor waits for the callback activation to complete. Both then stop processing all further messages, including timers. The awaits bought no ordering that the runtime does not already provide: messages from one actor to another are delivered in the order they were sent, so invoking the callbacks as plain asynchronous calls still delivers responses to each callback actor in request order. All user callbacks are now invoked without awaiting them, and the actors keep serving their connections while user callbacks run. Two pieces of server machinery silently relied on the blocking behavior and are reworked to stand on their own: - Server.cb_install parsed a request that had been buffered before the handler was installed and then dropped it. This was masked by the listener awaiting on_accept before installing the receive callback, which made early buffered data impossible as long as on_accept installed the handler synchronously. cb_install now dispatches buffered requests, and request parsing loops so that several requests pipelined into one TCP segment are all dispatched rather than only the first. - The respond closure handed to the request handler compared the live request counter instead of the id of its own request, which was only correct while the server was parked during the handler so that at most one request was in flight; a response to any concurrent request was silently dropped, with buffering left as a TODO. respond now captures its request id, and a response to a later request is buffered until the responses to all earlier requests have been sent, so responses go out on the connection in request order even when handlers respond out of order.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12b653309d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| send_response(status_code, headers, body) | ||
| response_count += 1 | ||
| # Send buffered responses that are next in line | ||
| while not close_connection and response_count + 1 in outstanding_responses: |
There was a problem hiding this comment.
When multiple requests arrive in one buffer and a later request specifies Connection: close, _process_buf sets the shared close_connection flag before any asynchronous handler can respond. The first response then closes the socket, and this condition also prevents buffered later responses from being flushed, so valid pipelined requests can lose every response after the first. Capture the close policy per request and close only after that request's ordered response has been written; the mirrored TLS path has the same issue.
Useful? React with 👍 / 👎.
1 participant
http.Client invoked user callbacks with an explicit await: _on_conn_connect did "await async on_connect(self)" and _on_con_receive did "await async cb(self, r)" for every parsed response. Awaiting parks the client actor until the user's callback activation has completed, so the client processes none of its own messages in the meantime. A callback that calls back into the client and waits for the result, for example by assigning the result of a post call, deadlocks both actors: the callback actor waits for the client to process its call, while the client waits for the callback activation to complete. Both actors then stop processing all further messages, including timers. The same pattern existed on the server side: http.Listener and http.TLSListener awaited the user's on_accept callback, and http.Server and http.TLSServer called the user's request handler in value position, which likewise waits for the handler activation to complete. A handler that awaits the respond callback, which is an action of the server actor, deadlocked the same way.
The awaits bought no ordering that the runtime does not already provide. Messages sent from one actor to another are delivered in the order they were sent, so invoking the callbacks as plain asynchronous calls still delivers each response to its request's callback in request order. All user callbacks are now invoked without awaiting them, and the actors keep serving their connections while user callbacks run. The listener also no longer stalls accepting further connections while one accept callback runs.
Two pieces of server machinery silently relied on the blocking behavior and are reworked to stand on their own. First, Server.cb_install parsed a request that had arrived before the handler was installed and then dropped it. This was masked by the listener awaiting on_accept before installing the receive callback on the accepted connection, which made buffered early data impossible as long as on_accept installed the handler synchronously; an on_accept that installs the handler later, for example after fetching configuration, would silently lose the first request. cb_install now dispatches requests buffered while no handler was installed, and request parsing loops, so several requests pipelined into a single TCP segment are all dispatched rather than only the first. Second, the respond closure handed to the request handler compared the live request counter instead of the id of its own request, which was only correct while the server was parked during the handler so that at most one request was in flight; a response to any concurrent request was silently dropped, with buffering left as a TODO in the code. respond now captures its request id, and a response to a later request is buffered until the responses to all earlier requests have been sent, so responses go out on the connection in request order even when handlers respond out of order.