fix(http-server): make stop() and destruction wait for listen() - #633
Merged
Conversation
andiwand
force-pushed
the
fix/http-server-teardown
branch
from
July 30, 2026 18:53
6473b35 to
6dda572
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6473b355bf
ℹ️ 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".
andiwand
force-pushed
the
fix/http-server-teardown
branch
2 times, most recently
from
July 30, 2026 19:12
b462bb0 to
457eefc
Compare
`stop()` and `~Impl` both dropped the `httplib::Server` right after asking it to stop, while the caller's thread was still inside `listen_after_bind()`. httplib only joins its own thread pool there - the thread that called `listen()` is the caller's, which it can neither know about nor join - so the accept loop was left standing on freed memory. That is the SIGSEGV in #631, and also the fdsan abort: reading `svr_sock_` out of the destroyed server hands `listen_internal()` a stale descriptor to close, which by then belongs to something else in the process. `listen()` now takes a reference to the impl and to the httplib server of its own, and `stop()` closes the socket and then waits for the accept loop to be out before releasing either. Nothing is serving once it returns, so there is nothing left to race - no join, no timeout, no guessing on the caller's side. Destroying the last handle does the same, which is what makes java's try-with-resources safe around a listen thread. Two orderings that were not handled at all: - `httplib::Server::stop()` closes nothing until `listen_internal()` has the accept loop up, and asserts (then closes a closed descriptor) if it runs a second time after that. A `stop()` that overtakes a starting `listen()` now waits for the loop rather than stopping twice or hanging. - A `listen()` that starts after `stop()` returns instead of serving a socket that is already gone. Also exposes `is_running()` - the state the caller had no way to ask about - in C++, java and python, and fixes the `~Impl` comment that claimed destroying the server joined the listen thread. Closes #631 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEvpyHCo15bkwhKgsEviGi
andiwand
force-pushed
the
fix/http-server-teardown
branch
from
July 30, 2026 21:07
457eefc to
d5d5924
Compare
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
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 #631.
What was wrong
Both crashes in the issue have the same root:
stop()and~Impldropped thehttplib::Serverright after asking it to stop, while the caller's thread wasstill inside
listen_after_bind().httplib joins its own thread pool there. The thread that called
listen()isthe caller's — httplib neither knows about it nor can join it — so the accept
loop was left standing on freed memory. That is the SIGSEGV.
It is also the fdsan abort, which I think is a consequence rather than a second
bug.
Server::stop()exchangessvr_sock_toINVALID_SOCKETbefore closingit, so the
close_socket(svr_sock_)on the way out of the accept loop readsINVALID_SOCKETand does nothing — as long as the server is still there to readfrom. Once it has been freed, that read returns whatever the allocator left
behind, and
listen_internal()closes a descriptor that by then belongs tosomething else in the process. Hence
owned by ZipArchive.The fix
listen()takes a reference to the impl and to the httplib server of its own,and
stop()closes the socket and then waits for the accept loop to be outbefore releasing either. Nothing is serving once
stop()returns, so there isnothing left to race — no join, no timeout, no guessing on the caller's side.
Destroying the last handle does the same, which is what makes
try (HttpServer s = new HttpServer())safe around a listen thread.Two orderings that were not handled at all:
httplib::Server::stop()closes nothing untillisten_internal()has theaccept loop up, and asserts (then closes an already closed descriptor) if it
runs a second time after that. A
stop()that overtakes a startinglisten()now waits for the loop rather than stopping twice or hanging.
listen()that starts afterstop()now returns instead of serving asocket that is already gone.
The public header is unchanged apart from
is_running()— no new member, no newtype. A
HttpServerholds a secondshared_ptrto the same impl whose deleterstops the server instead of destroying it, with the owning reference captured in
the deleter.
shared_from_this()stays bound to the control blockmake_sharedput there (a later one only takes
weak_thisover once it has expired), so alisten()in flight holds that reference and does not keep the handles alive.Running out of handles therefore stops, and
~Implonly tidies up after it.Also exposes
is_running()— the state the caller had no way to ask about — inC++, java and python, and fixes the
~Implcomment that claimed destroying thehttplib server joined the listen thread.
Bindings
java:
isRunning(), and a newGuardedNativeResource— aNativeResourcewhose handle stays valid for the duration of a native call even if another
thread closes it. Stopping alone is not enough: a listen thread that has read
the handle but not reached
listenNativeyet is invisible to the nativestop(), soclose()also waits for the call to hand the handle back, havingfirst called
unblock()(stop()here) to make it come back at all.It is a separate base class rather than a change to
NativeResourcebecausethe hazard is caller error everywhere else — you do not use an object while
you close it — and
listen()is the one call the API asks you to make on athread of its own. Classes that don't need it keep the plain
handle(), withno extra fields, no lock, and nothing on
Element's 34 native calls.python:
is_running(), docstrings for the newlisten/stopcontract.stopalready released the GIL, which it now needs to: it waits for thelisten thread, which needs the GIL back to return out of
listen.Tests
New gtest cases:
stopreturns only oncelistenhas,stopbefore the acceptloop is up,
stoptwice, destroying the last handle while listening,listenafter
stop. Mirrored in the JUnit and pytest suites, plus acloseStopsListenandroid instrumented test — fdsan there is what turns a double close into a hard
abort rather than a silent one.
Verified against the old
stop()body:stop_returns_only_once_listen_hasfails on it. Ran locally on macOS — gtest (16k iterations of the race cases),
the JNI JUnit suite and pytest.
What is not fixed
A server that was bound but never listened still keeps its port until the
process ends: cpp-httplib closes the listening socket only from its accept loop
and exposes no way to reach it. That was already documented on
stop()andstill is.