fix(http1): close a finished connection instead of holding it until reap

A connection's socket was owned by the registry entry and only released
when the next accept() reaped it, so a peer we had finished with — after
a 408, a 400, or a `connection: close` — never saw EOF and sat waiting
for a server that was done talking. On a server that goes quiet it also
held every descriptor from the last burst indefinitely.

The connection thread now closes its own socket the moment Serve()
returns, under the registry lock so Stop()'s shutdown() can never name a
descriptor that has already been released, and Stop() waits on a
condition variable for the last thread rather than assuming the vector
it moved out is quiescent. Adopt() is also fully guarded: it runs on
ListenerTCP's accept loop, which has no handler, so anything escaping it
would abort the process.

Found by ShouldSurviveAbuseHTTP1, added here: 24 concurrent keep-alive
clients, peers that vanish mid-request or send garbage, and a peer that
stalls forever — the server must keep serving and still stop promptly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
catbot 2026-07-27 00:56:53 +00:00
commit ea1310faec
3 changed files with 159 additions and 4 deletions

View file

@ -65,6 +65,9 @@ struct ListenerHTTP1::Impl {
ListenerHTTP1* owner = nullptr;
std::unique_ptr<ListenerTCP> listener;
std::mutex mutex;
// Signalled when a connection thread finishes, so Stop() can wait for
// the last one instead of polling.
std::condition_variable idle;
std::vector<std::unique_ptr<HTTP1Connection>> connections;
std::atomic<bool> running{true};
std::atomic<std::uint64_t> accepted{0};
@ -79,7 +82,10 @@ struct ListenerHTTP1::Impl {
});
}
void Adopt(ClientTCP* accepted) {
void Adopt(ClientTCP* accepted) try {
// The unique_ptr takes ownership immediately, so every path out of
// here — including the shutdown early-return and a throwing thread
// constructor — closes the socket.
auto connection = std::make_unique<HTTP1Connection>();
connection->client.reset(accepted);
HTTP1Connection* pointer = connection.get();
@ -94,9 +100,24 @@ struct ListenerHTTP1::Impl {
} catch (...) {
// A connection dying must never take the server with it.
}
pointer->finished.store(true);
{
std::lock_guard lock(mutex);
// Close here rather than when the entry is reaped: the peer
// has to see the connection end as soon as we are done with
// it, and holding the descriptor until the next accept
// would be an unbounded leak on a server that goes quiet.
// Doing it under the lock keeps Stop()'s shutdown() from
// ever touching a descriptor number we have released.
pointer->client.reset();
pointer->finished.store(true);
}
idle.notify_all();
});
connections.push_back(std::move(connection));
} catch (...) {
// This runs on the accept loop, which has no handler of its own:
// letting anything escape (a thread that could not be spawned, say)
// would abort the process.
}
void Send(ClientTCP& client, const std::string& wire) {
@ -261,12 +282,18 @@ void ListenerHTTP1::Stop() {
std::vector<std::unique_ptr<HTTP1Connection>> closing;
{
std::lock_guard lock(impl->mutex);
std::unique_lock lock(impl->mutex);
for (auto& connection : impl->connections) {
// Wake the serving thread out of poll()/recv() without closing
// the descriptor underneath it.
// the descriptor underneath it — the thread owns that.
if (connection->client) shutdown(connection->client->socketid, SHUT_RDWR);
}
impl->idle.wait(lock, [&] {
return std::ranges::all_of(impl->connections,
[](const std::unique_ptr<HTTP1Connection>& connection) {
return connection->finished.load();
});
});
closing = std::move(impl->connections);
impl->connections.clear();
}