test(http-server): guard the prompt stop after a kept-alive request - #713
Merged
Conversation
A kept-alive connection used to hold the accept loop until it idled out, so `stop()` spanned cpp-httplib's 5 s keep-alive timeout — on the path every consumer takes, potentially on the main thread while a document closes. The 0.47.0 bump (#653) fixed it upstream: the keep-alive wait now watches the listening socket, so closing it ends the wait. Nothing said so, so nothing would notice it coming back. This serves a request from a client that keeps the connection open, then times `stop()`: 12 ms here, against the 5.01 s the issue measured. `odr_test` links `httplib::httplib` for the client. Closes #641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XDs5aK3ZGSZsEvqUUwBBXU
![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: f4216e6cbb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`ASSERT_TRUE(response)` returns from the test with the listen thread still joinable, and `~std::thread` then calls `std::terminate` — a transient socket error would abort the whole binary instead of reporting one failing test. A guard now stops the server and joins on every path; a second `stop()` is harmless, as the test above it says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XDs5aK3ZGSZsEvqUUwBBXU
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Generated with Claude Code
Closes #641 — already fixed, this is the test that says so.
#641 reported
HttpServer::stop()blocking ~5 s after any request had been served: the accept loop sat in cpp-httplib's keep-alive wait until the client's connection idled out, andstop()waits forlisten()to return (rightly — that wait is what makes destruction safe, #633).The bump to cpp-httplib 0.47.0 in #653 fixed it upstream. In 0.47.0
detail::keep_alive()polls the listening socket and breaks as soon as it is closed, soServer::stop()ends the wait instead of outliving it. That PR measured the same thing from the other side: "serving a rendered view takes 5.03 s on 0.16.3 and 0.03 s on 0.47.0".Nothing in the suite covered it, so nothing would notice it coming back — a dependency bump or a
set_keep_alive_timeoutcall could reintroduce it silently. This adds the missing coverage:stop_is_prompt_after_serving_a_kept_alive_requestserves a request from anhttplib::Clientthat keeps the connection open (and stays alive across thestop(), so the connection really is open there), then timesstop().odr_testlinkshttplib::httplibfor the client.Verified
12 ms against the 5.01 s the issue measured; the bound is 2 s, well clear of both.