Repository navigation
HttpServer: add a minimal Qt-free HTTP/1.1 server - #1879
Conversation
pa-devbot
left a comment
pa-devbot
left a comment
There was a problem hiding this comment.
AI review: Overall: clean, well-documented module, and the 10 loopback tests are genuinely good (including the hard cases: disconnect during a handler, stop() during a handler, restart, second-client rejection). I built the branch and ran the full suite locally: 20,458 passed, 0 failed, 5 skipped (pre-existing) — matches the PR description.
I did find two deadlocks and one busy-spin, all around m_lock:
1. Deadlock: on_connect() waits for the previous connection task while holding m_lock (comment on HttpServer.cpp:258). The move-assignment destroys the previous task's AsyncTask handle; the destructor blocks until that task finishes (~AsyncTask_Cpp, Common/Cpp/Concurrency/Backends/AsyncTask_Default.h), and the previous task's final cleanup takes the same m_lock (line 413). A new client connecting while the previous task is finishing → on_connect holds the lock waiting for the task, the task waits for the lock → permanent deadlock.
I reproduced it: I widened the final-cleanup window with a 500 ms sleep (local-only, uncommitted), reconnected immediately after the server closed a Connection: close response, and the whole test run hung. Thread trace: on_connect blocked in the handle assignment while holding m_lock; previous task blocked on m_lock at line 413; the test thread's stop() then queued on m_lock and never returned. Without the artificial widening the window is just the task's final lock section — narrow, but the same cycle. This is exactly the pattern the socket layer deliberately avoids: ServerSocket_POSIX.cpp:368 ("Start the connection thread without holding m_lock ... that task may be blocked on m_lock") and the m_task_lock comments in the socket headers.
Related liveness issue, same root: when a new client connects while the previous handler is still running (the documented "tool still running" case), the new connection's on_connect blocks until the old handler returns, so no task gets dispatched for the new client in the meantime.
Suggestion: dispatch/assign outside m_lock (a separate task lock, like the sockets' m_task_lock), and/or detach the old handle and wait for it in stop().
2. Deadlock: stop() destroys the socket while holding m_lock (comment on HttpServer.cpp:228). m_socket = nullptr runs ~ServerSocket → close_and_wait(), which joins the socket's connection thread — the thread that calls on_connect(), which takes the very m_lock stop() is holding during destruction. A client connecting while stop() runs (the socket keeps accepting while LISTENING) leaves the connection thread blocked on m_lock and the join inside the destructor never returns, so stop() — and therefore ~HttpServer — hangs forever. Same root cause: holding m_lock across a blocking task join. Suggestion: move the unique_ptr out under the lock and destroy it after releasing it.
3. Busy-spin while a partial request is in the buffer (comment on HttpServer.cpp:344). The wait predicate is !conn->buffer.empty() || .... Once the client has sent any part of an incomplete request, the predicate is immediately true, so wait_for returns without waiting and the connection task tight-loops in try_parse at 100% CPU until the request completes, the peer disconnects, or stop(). (I verified the CV behavior directly: partial request in the buffer → wait_for(10 ms, pred) returns in ~0 µs; empty buffer → honors the 10 ms.) A stalled or slowly trickling client pins one pool thread indefinitely. Suggestion: record buffer.size() at parse time and only wake on size change / dead / stop.
Minor (inline): 411 vs 501 for chunked, an unreachable 413 check, a missing "start/stop are not concurrent" doc note, and a stale "same port" comment in a test.
| auto conn = std::make_shared<Connection>(); | ||
| m_connection = conn; | ||
| try{ | ||
| m_connection_task = m_thread_pool.dispatch( |
There was a problem hiding this comment.
AI review: Deadlock risk: this move-assignment destroys the previous connection task's AsyncTask handle while m_lock is held, and the destructor blocks until that task completes. The previous task's final cleanup takes the same m_lock (line 413) — a new client connecting while the previous task is finishing deadlocks: on_connect holds the lock waiting for the task, the task waits for the lock. Reproduced (with the final-cleanup window artificially widened — see review body); without it the window is the task's final lock section, narrow but the same cycle. This is the pattern ServerSocket_POSIX.cpp:368 explicitly avoids (dispatch outside m_lock, dedicated m_task_lock for task slots). Also note the liveness case: a new client connecting while the previous handler is still running makes on_connect block until that handler returns, with no task dispatched for the new client in the meantime.
|
|
||
| { | ||
| std::lock_guard<Mutex> lg(m_lock); | ||
| m_socket = nullptr; |
There was a problem hiding this comment.
AI review: m_socket = nullptr runs ~ServerSocket → close_and_wait() while m_lock is held, and that join includes the socket's connection thread — which calls on_connect() and takes this same m_lock (line 250). If a client connects while stop() is running (the socket keeps accepting while LISTENING), the connection thread blocks on m_lock and the join inside the destructor never returns, so stop() — and ~HttpServer — hang. Same root cause as the on_connect issue: holding m_lock across a blocking task join. Suggestion: move the unique_ptr out under the lock, destroy it after releasing the lock.
| { | ||
| std::unique_lock<Mutex> lg(conn->lock); | ||
| conn->cv.wait_for(lg, std::chrono::milliseconds(10), [this, &conn]{ | ||
| return !conn->buffer.empty() |
There was a problem hiding this comment.
AI review: Busy-spin: once a partial request is in the buffer, !conn->buffer.empty() is immediately true, so wait_for returns without waiting and the loop tight-spins in try_parse at 100% CPU until the request completes, the peer disconnects, or stop(). A stalled or slowly uploading client pins one pool thread indefinitely. Suggestion: record buffer.size() at parse time and wake only when it changes (or on dead/stop).
|
|
||
| // Special headers. | ||
| if (result.request.headers.count("transfer-encoding") != 0){ | ||
| error(411, "Chunked transfer encoding is not supported."); |
There was a problem hiding this comment.
AI review: Minor: RFC 9112 suggests 501 for a transfer coding the server cannot handle. 411 ("Length Required") is defensible here since you are demanding Content-Length, but 501 is the more standard status for "chunked not supported".
|
|
||
| const size_t consumed = header_end + 4 + (size_t)content_length; | ||
| if (buffer.size() < consumed){ | ||
| if (buffer.size() > MAX_BUFFER_BYTES){ |
There was a problem hiding this comment.
AI review: Unreachable: at this point any request that can be in NEED_MORE has content_length <= 1 MiB (larger already got the 413 above) and header_end <= ~64 KiB, so consumed < 2 MiB and buffer.size() < consumed can never exceed MAX_BUFFER_BYTES. Harmless, but consider dropping it or noting it is a defensive check.
| * Threading: | ||
| * - All "Listener" callbacks run on threads from the thread pool, never | ||
| * on the calling thread. | ||
| * - Do not call "start()" or "stop()" from within a "Listener" callback: |
There was a problem hiding this comment.
AI review: Consider also documenting that start() and stop() must not be called concurrently from different threads: start() captures the raw socket pointer under the lock and then calls socket->start() outside of it, so a concurrent stop() could destroy the socket mid-call.
| } | ||
|
|
||
| if (error.empty()){ | ||
| // Start again on the same port and serve a fresh request. |
There was a problem hiding this comment.
AI review: Nit: start_server() actually picks a new random port on each call (port is out-only) — the comment says "the same port".
981e229 to
f3b9f84
Compare
pa-devbot
changed the title
f3b9f84 to
597409a
Compare
pa-devbot
left a comment
pa-devbot
left a comment
There was a problem hiding this comment.
AI review: Re-review of the updated branch: all three original issues are fixed as suggested, and I verified each fix empirically.
1. on_connect() deadlock — fixed. Task handles now live in m_tasks under a dedicated m_task_lock (the socket library's pattern); dispatch happens outside m_lock, and finished handles are reaped at the next connection while unfinished ones are kept. Verified: I re-ran my original repro — a 50-iteration close + immediate-reconnect churn, without any artificial window widening — and it now passes cleanly (before, it deadlocked even with the window widened).
2. stop() socket-destruction deadlock — fixed. The socket is moved out under m_lock and destroyed after the lock is released, so ~ServerSocket's join of the socket's worker threads (whose callbacks take m_lock) no longer races the lock. Verified by code inspection.
3. Busy-spin — fixed. The wait predicate now requires the buffer to have grown past what the parse just saw (buffer_size captured under conn->lock). Verified by measurement: with a partial request sitting in the buffer for 20 s, the whole test process (108 threads) used 1.3% of one core and every thread was sleeping — with the old predicate the connection task would have spun at ~100%.
Minor items — all addressed: 411 → 501 (with an RFC 9112 comment); the unreachable 413 check is gone and the cap is now enforced when data is appended, which also closes the old transient unbounded-growth between receives, with a new Test_Http_BufferCap test to match; the "same port" test comment is fixed; and the start()/stop()-are-not-concurrent note now lives in the AbstractHttpServer interface.
Full suite on the rebased branch: 20,528 passed, 5 skipped. The only 2 failures are PokemonSV::ItemPrinterJobsDetector image tests failing in file_to_string — my workspace's CommandLineTests checkout is missing those images (added upstream with #1890/#1891, after this daily-updated checkout was cut). Unrelated to this PR.
One new residual issue (narrow, low priority — see inline comment): the old connection's task can still write its final response into the socket after it has been handed to the next client. The new end_connection guard correctly stops the old task from closing the new client's socket, but write_response/socket->send() has no per-connection guard. The window is microscopic (a reconnect must be accepted between the old socket thread's state transition and its on_disconnect call), so this does not block — but it is the same class of "task outlives the connection" issue, and the suggested fix (flag the replaced connection dead in on_connect, and/or re-check before writing) is cheap.
| } | ||
| } | ||
|
|
||
| // The connection may have closed while the handler was running; |
There was a problem hiding this comment.
AI review: Residual narrow race (low priority, not blocking): this check guards the close, but not the write. on_disconnect (which sets dead) runs on the socket's connection thread after that thread has already set the state to LISTENING — so a new client can be accepted, and the socket's active fd re-pointed at the new connection, before conn->dead becomes true. If the old task's handler returns in that window, this check sees dead == false and write_response → socket->send() delivers the old connection's final response into the new client's socket. The new end_connection guard correctly stops the old task from closing the new client's connection, but the write is unguarded. The window is microscopic (a reconnect accepted between the old socket thread's state transition and its on_disconnect call), so this does not block the PR. Suggestion: in on_connect, when replacing m_connection, set the old connection's dead flag under m_lock (and notify its cv); and/or re-check conn->dead immediately before write_response. A fully robust fix would need per-connection identity in AbstractServerSocket::send(), which is out of scope here.
|
The fresh reviewer found five actionable bugs and recommends fixing them before merging.
|
30bfa48 to
4b53dbf
Compare
4b53dbf to
a241b0f
Compare
…cket A minimal HTTP/1.1 server for a single local client that issues one request at a time (e.g. an MCP client), built on the socket library instead of Qt: - One connection at a time (the server socket's model); requests are processed in order, so pipelined bytes simply wait in the buffer. - The per-connection task does all parsing and writing; the socket's receive callbacks only append data and flip flags. The user handler may block on another thread (e.g. the program thread) and can poll HandlerContext::connection_alive() to bail out of long-running work; responses are discarded if the connection dies meanwhile. - Content-Length bodies only (chunked -> 411), sizes capped (8 KB request line, 64 KB headers, 1 MiB body, 2 MiB buffer), "Expect: 100-continue" answered, strict request/header validation (400), responses always carry Content-Length. - start()/stop() are fully synchronous from the caller's point of view once on_listen_finished has fired: stop() closes the connection, waits for the in-flight handler, and joins the socket's worker threads, so the server can be started again afterwards. No TLS, no upgrades, no method routing or auth: the user handler owns all of that. 10 loopback tests (raw ClientSocket clients) cover the happy path, keep-alive, pipelining, Connection: close, 400/411/413, 100-continue, disconnect and stop() during a handler, and second-client rejection. Part of the groundwork for the in-app MCP server; stacks on the ServerSocket factory alias PR.
- Rename ParseState::ERROR to ParseState::ERROR_RESPONSE: ERROR is a macro on Windows (winerror.h), which broke the qualified name on MSVC and clang-cl (C2589). - Keep the AsyncTask handle from ThreadPool::dispatch in the stop-during-handler test: dispatch is [[nodiscard]] and clang's -Wunused-result (CI's -Werror) rejects discarding it. Its destructor also waits for the trivial release task to finish.
Split the request/response types, the handler contract, and the listener events out into AbstractHttpServer (stacked on the extract-abstract-http-server PR); HttpServer now implements that interface, with the types re-exported under its own name. Fixes from the review: - Deadlock in on_connect(): the connection task handle was moved into m_connection_task while holding m_lock, and destroying the old handle blocked on the old task, whose final cleanup takes m_lock. Task handles now live in m_tasks under a dedicated m_task_lock (the socket library's m_task_lock pattern): on_connect() dispatches outside m_lock and never waits on an old task, so a new client is no longer held up while the previous handler is still running. - Deadlock in stop(): the socket was destroyed while holding m_lock, and ~ServerSocket joins the socket's worker threads, whose callbacks take m_lock. The socket is now moved out under m_lock and destroyed after releasing it. - Busy-spin on partial requests: the wait predicate was !buffer.empty(), which is permanently true once a partial request is buffered. It now requires the buffer to have grown past what the parse just saw, plus the dead/stop flags. - Chunked requests: 501 Not Implemented instead of 411 (RFC 9112). - The 413 check at parse time was unreachable (an incomplete request is always smaller than the buffer cap); the 2 MiB buffer cap is now enforced when data is appended instead, closing the connection. - end_connection() only closes the socket's connection while it still belongs to this connection, so a task noticing its own connection died late cannot close the next client's connection. - Documented that start()/stop() are not thread-safe with each other. Test: new case that a connection flooding past the buffer cap is closed without a response while the server keeps listening; chunked case now expects 501.
The socket's connection thread sets the state back to LISTENING before it reports the disconnect, so a fast reconnect can be accepted before the old connection's on_disconnect callback runs. Two consequences: - The old connection's task could then write its final response through socket->send() into the new client's socket (send() has no per-connection identity; it writes whatever the socket is connected to). on_connect() now marks the connection it replaces dead (and notifies it) before the new one takes over, so the old task's post-handler check discards its response. - on_disconnect() marked m_connection dead, which in that window is the new client's connection: it would kill the new connection task. It now only does so while the socket is not already CONNECTED to the next client. Test: a client disconnects while its handler is still blocked on a gate; a second client connects (replacing the first in the server) before the gate is released. The first handler's late response must be discarded, and the second client's stream must contain only its own response.
Five items from the fresh review: - [P1] stop() could hang waiting for a handler: the socket suppresses its disconnect callback for a close_connection() close, so a handler polling connection_alive() kept seeing true while shutdown waited for it. stop() now marks the active connection dead before it waits. The stop-during-handler test now polls connection_alive() after the gate, which would spin forever without the mark. - [P1] A response could reach a different client after reconnection: every write now carries the connection's socket id (AbstractServerSocket::connection_id(), new), so the socket refuses a write whose connection was replaced. This closes the last of the 'task outlives the connection' windows; the id check and the transport selection happen under the socket's lock. - [P2] Expect: 100-continue stalled clients: the interim response was sent only after the complete request (including body) was parsed, which a client waiting for it before sending the body never sees. Parsing now reports a separate NEED_BODY state (headers complete and valid, body pending), and the 100 is answered at that point. The test now performs the real two-phase handshake. - [P2] Complete headers bypassed the 64 KiB limit: the in-progress check only fires while the terminator is still missing. The completed header boundary is now checked against the limit too, with a test sending a ~75 KiB header section in one piece. - [P2] The test client's receive buffer was a data race: 'received' and the error strings are now protected by the waiter's mutex, and assertions use synchronized snapshots (same for the server listener's listen error).
a241b0f to
682e922
Compare
Adds
Common/Cpp/HttpServer/HttpServer.{h,cpp}— a minimal HTTP/1.1 server onAbstractServerSocket(the platform default server socket), plus ~1,000 lines of loopback tests driven by rawClientSocketclients. No Qt anywhere; the module lives inCommon/Cppnext to the socket library it builds on.Stacked on the two socket PRs (both already in main): the
ServerSocketfactory alias (#1878) andAbstractHttpServer(#1884).HttpServerimplements theAbstractHttpServerinterface — the request/response types, the handler contract, and the listener events live on the abstract class; the concrete class re-exports them under its own name, so existing call sites keep writingHttpServer::Requestetc.Design: one connection at a time (a second client is rejected), requests processed strictly in order, one per-connection task from the thread pool does all parsing/writing so handlers may block (e.g. running a tool on the program thread) without stalling the socket.
HandlerContext::connection_alive()lets long handlers bail out; the response is discarded if the connection died.Content-Lengthbodies only (chunked → 501), with caps: 8 KB line, 64 KB headers, 100 headers, 1 MiB body, 2 MiB per-connection receive buffer (connection closed when exceeded).stop()is fully synchronous: close connection, wait for the in-flight handler, join the socket threads;start()works again afterwards.Threading invariants (from the review): task handles live in
m_tasksunder a dedicatedm_task_lock(the socket library's pattern), soon_connect()never destroys a handle while holdingm_lockand never blocks on a previous connection's still-running handler;stop()destroys the socket after releasingm_lock(its destructor joins worker threads whose callbacks takem_lock).Tests: 11 loopback cases — POST echo, pipelined keep-alive, Connection: close, malformed request line, chunked rejection (501), oversized content-length (413), receive-buffer cap, Expect: 100-continue, disconnect and stop() during a handler, and second-client rejection.
Part of the groundwork for the in-app MCP server (Streamable HTTP transport); the JSON-RPC/MCP protocol layer and ProgramSession integration are follow-ups.
Note (2026-11-12): the socket change (per-connection send id) was split into #1910 at the maintainer's request and is now merged into main. This PR is rebased on main; its diff is HttpServer-only.