Repository navigation
Fix crash on server shutdown racing HTTP workers - #681
Merged
Merged
Conversation
The SIGTERM/SIGINT handler called server.stop() from the main thread while the HTTP worker threads were still running. stop() closes the acceptor and every open connection socket, and asio objects must not be used concurrently: an accept handler re-arming async_accept on a worker thread could see the acceptor closed underneath it and crash in epoll_reactor::start_op, and Connection::close() could race with the accept operation assigning that connection's socket. This surfaced as an intermittent "-11 != 0" in test_shutdown_1_term_multiple_threads. Stop only the io_service from the signal handler, which is thread-safe, and close the acceptor and the connections after the workers have been joined. Add a shutdown case that keeps clients connecting while the signal arrives, so accept handlers are in flight during shutdown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Connect until stopped instead of a fixed count, and send the signal only after a connector has been accepted, so clients are still connecting when shutdown starts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Fixes the intermittent
test_shutdown_1_term_multiple_threadsfailure (AssertionError: -11 != 0) on master: master run 40606635554 (b7f51b0f, Protein Debug) and 40110252632 (98291431, Protein Release static noAVX). It is not caused by #671, which only changestests/test_all.cpp.Root cause
The SIGTERM/SIGINT handler added in #675 calls
server.stop()on the main thread while the HTTP worker threads are still runningio_service->run().stop()closes the acceptor and shuts down every open connection socket. Asio sockets must not be used concurrently, so:acceptor->async_accept(server_http.hpp:859) while the main thread runsacceptor->close().epoll_reactor::start_opthen dereferences the released descriptor state, which is the SIGSEGV;Connection::close()(socketshutdown/cancel) races with the accept operation assigning that connection's socket (do_assign).The test sends an incomplete request right before the signal, so a just-accepted connection's handlers are often in flight at that moment.
Fix
The signal handler now only calls
io_service->stop(), which is thread-safe. The acceptor and the connections are closed withserver.stop()afterserver_thread.join(), when no worker is left. Shutdown behaviour is otherwise unchanged: in-flight handlers still finish, and an incomplete request still does not keep a worker alive.A new
term_while_connectingcase keeps clients connecting (RST on close, capped, so it leaves no TIME_WAIT) while the signal arrives, so accept handlers are in flight during shutdown.Evidence
acceptorclose vsasync_accept,shutdownvsdo_assign, …). AddressSanitizer reports a SEGV inepoll_reactor::start_op←async_accept← the accept handler, with both-p 4and-p 1.-p 4and 2000/2000 with-p 1.b7f51b0f:rc=-11) in about 250 runs.test_server_shutdown.pypassed 6/6 runs, 7/7 tests each.Not run locally: the unit tests and the rest of the integration suite (left to CI). No unit test covers
server.cpp.🤖 Generated with Claude Code
Summary by cubic
Fixes the intermittent crash on server shutdown where SIGTERM/SIGINT raced with running HTTP workers. The signal handler previously called
server.stop(), which closed the acceptor and all connection sockets from the main thread while accept and read handlers were still running on worker threads, occasionally crashing in asio.server.stop()is called after the worker threads have been joined.term_while_connectingshutdown test that keeps clients connecting while the signal arrives and only sends the signal after the first connection is accepted, so accept handlers are in flight during shutdown.Shutdown behavior is unchanged otherwise: in-flight handlers still finish and an incomplete request still does not keep a worker alive.
Written for commit f52876b. Summary will update on new commits.