client: drain workers concurrently at shutdown#1559
Merged
Conversation
ProcessImpl::shutdown() signaled and joined each worker in turn, so the per-worker connection pool drains (each capped by --timeout when the target has stopped answering) ran strictly one after another: total shutdown time scaled with concurrency x timeout. On a many-core machine with auto concurrency and a wedged target this presents as nighthawk failing to shut down at all. Split worker shutdown into an idempotent initiateShutdown() plus a join, and have ProcessImpl::shutdown() signal every worker before joining any, so all drains overlap and shutdown is bounded by ~1x timeout regardless of concurrency. FlushWorkerImpl keeps its existing behavior and ordering. Also guard the join with joinable(): shutdown() also runs on early-exit paths where workers were created but never started (e.g. cluster manager initialization failure), where joining the never-started std::thread would throw. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
687a91f to
23198a9
Compare
eric846
approved these changes
Jul 9, 2026
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.
Part 2/3 of fixing nighthawk failing to shut down when a target can't keep up.
Symptom: shutdown time scales with concurrency: each worker's connection pool drain (up to
--timeoutwhen the target has stopped answering) ran strictly after the previous worker's, so e.g. 3 workers x--timeout 2measured 6.05s of shutdown wall clock before this change, ~2s after. With--concurrency autoon a many-core machine and a wedged target this presents as nighthawk failing to shut down at all.Fix:
Worker::initiateShutdown()(new pure virtual, implemented once inWorkerImpl; no other implementers exist) separates the exit signal from the join.ProcessImpl::shutdown()signals all workers, then joins them, so the drains overlap.FlushWorkerImplkeeps its existing behavior and ordering. Also guards the join withjoinable(): shutdown() also runs on early-exit paths where workers were created but never started (e.g. cluster manager initialization failure), where joining the never-startedstd::threadwould throw.Testing: new
ShutdownDrainsWorkersConcurrentlytest with a wedged TCP server (accepts, never responds) asserts shutdown < 2x timeout with a >= 0.75x lower bound guarding against vacuous passes; green on IPv4+IPv6.Reviewer note: this makes per-worker teardown (
terminate()drain +tls_.shutdownThread()) run concurrently across workers, which Envoy's own serial worker shutdown never exercises. The pinned Envoy internals on that path were reviewed (thread-local data is per-thread, stat updates are atomic or behind the locked central cache aftershutdownThreading(), slot teardown touches per-thread containers) and no shared mutable state was found; asan/tsan runs pass. Flagging for extra scrutiny regardless.