Skip to content

client: drain workers concurrently at shutdown#1559

Merged
eric846 merged 1 commit into
mainfrom
shutdown-concurrent-worker-drains
Jul 9, 2026
Merged

client: drain workers concurrently at shutdown#1559
eric846 merged 1 commit into
mainfrom
shutdown-concurrent-worker-drains

Conversation

@oschaaf

@oschaaf oschaaf commented Jul 9, 2026

Copy link
Copy Markdown
Member

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 --timeout when the target has stopped answering) ran strictly after the previous worker's, so e.g. 3 workers x --timeout 2 measured 6.05s of shutdown wall clock before this change, ~2s after. With --concurrency auto on 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 in WorkerImpl; no other implementers exist) separates the exit signal from the join. ProcessImpl::shutdown() signals all workers, then joins them, so the drains overlap. FlushWorkerImpl keeps its existing behavior and ordering. Also guards 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.

Testing: new ShutdownDrainsWorkersConcurrently test 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 after shutdownThreading(), slot teardown touches per-thread containers) and no shared mutable state was found; asan/tsan runs pass. Flagging for extra scrutiny regardless.

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>
@oschaaf oschaaf force-pushed the shutdown-concurrent-worker-drains branch from 687a91f to 23198a9 Compare July 9, 2026 11:28
@eric846 eric846 merged commit 6dc0a67 into main Jul 9, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants