Shard the HttpProxy shutdown Notify to cut lock contention - #969
Shard the HttpProxy shutdown Notify to cut lock contention#969nbarbier-265 wants to merge 2 commits into
Conversation
Every request that parks in read_request() registers a shutdown waiter on a single Notify shared by the whole proxy, and unregisters it when the read completes. Both operations serialize on the Notify's internal mutex, which becomes the scaling bottleneck on many-core machines: cloudflare#844 measured ~65% of off-CPU time in futex waits on a 128-core NUMA host. Shard the Notify by worker thread so waiter registration stays on a core-local, cache-line-padded shard. http_cleanup() notifies all shards. Streams whose headers are already buffered never touch the Notify at all because the biased select polls read_request() first. This also closes a lost-wakeup race: handle_new_request() never checked shutdown_flag, so a notify_waiters() that fired between read_request() returning Pending and the waiter registering was missed and the connection lingered until the grace period expired. The shutdown arm now registers the waiter first, then checks shutdown_flag before awaiting. Fixes cloudflare#844
h2 0.3.27 comes in through the same legacy aws chain as the existing rustls-webpki ignores: dial9-tokio-telemetry -> aws-sdk-s3-transfer-manager -> aws-config -> aws-smithy-http-client, which still uses hyper 0.14. The advisory's only fix is h2 >= 0.4.16 and no 0.3.x patch exists, so this cannot be resolved from this workspace's manifests. The vulnerable code needs a malicious HTTP/2 peer; in this chain h2 is only a TLS client to AWS endpoints. Every CI run has failed the cargo audit step since the advisory was published on 2026-08-17.
1484ea6 to
da7ac62
Compare
xhon-pelushi
left a comment
There was a problem hiding this comment.
Reviewed the proxy change by building it and trying to break it. The concurrency work looks correct to me and I could not falsify it; the second commit is a different matter and I'd drop it.
The lost-wakeup fix is real, and the test catches it
Transplanted only the two new tests onto current main (09696b5), leaving the lib.rs change out:
test tests::shutdown_wakes_parked_read_requests ... ok
test tests::shutdown_before_read_request_parks_returns_immediately ... FAILED
panicked at pingora-proxy/src/lib.rs:2375: read_request parked after shutdown: Elapsed(())
So the race is reachable on main and the regression test does pin it, exactly as described. shutdown_wakes_parked_read_requests passes on main too, which matches your framing of it as a guard against the flag-only fix in #844 rather than a demonstration of the bug.
On PR head da7ac62, cargo test -p pingora-proxy --lib is 41 passed, 0 failed.
I also wrote an independent adversarial harness rather than trusting the two supplied tests — 300 rounds × 8 tasks per round, with the http_cleanup() call jittered by 0–6 yield_now()s to land inside the window between read_request() returning Pending and the waiter registering:
| result | |
|---|---|
main + harness |
fails at round 0 — "a parked read never woke" (10s timeout) |
| PR head + harness | 300/300 rounds pass |
The ordering argument holds
I checked it rather than taking it on trust, because this is the kind of claim that is usually subtly wrong:
http_cleanup()isshutdown_flag.store(true, Release)thenshutdown.notify_waiters().await_shutdown()isnotified.enable()(which takes the shard's mutex) thenshutdown_flag.load(Acquire).
Because ShardedNotify::notify_waiters() locks every shard, both sides serialise on the same shard mutex whichever way the race falls. If enable() gets the lock first the waiter is in the list and is woken; if it gets it after, the notifier's unlock of that same shard release-syncs with enable()'s acquire, and the store is sequenced before that unlock, so the load sees true. Sharding doesn't weaken this, since the argument only ever needs the one shard the waiter picked.
Two smaller things I checked and found fine:
- Task migration can't lose a wakeup:
self.shutdown.local()is evaluated once and theNotifiedstays bound to that shard, so a work-steal after registration is harmless. - Shard math is sound at the edges:
.min(256)is applied afternext_power_of_two(), so the count stays a power of two, and the degenerate 1-shard case indexes& 0. - Sibling sites: the only other
shutdown_flagconsumer isSession::is_process_shutting_down(), a read-only accessor. There's no second parking site carrying the same race, so the fix is complete.
Please drop da7ac62 ("Ignore RUSTSEC-2026-0258 in cargo audit")
It's now redundant, and merging it would quietly weaken a security note.
main already ignores that advisory — e819abf, "Temporarily ignore h2 advisory for optional Dial9 S3 support", landed 2026-08-24, four days after da7ac62 (2026-08-20):
# h2 0.3.27 is pulled in only by the optional worker-s3 dependency chain.
# The feature is disabled by default, but users who enable it remain affected.
"RUSTSEC-2026-0258", # h2: unbounded empty DATA framesThis PR's version of the file replaces that with a blanket claim in the header comment — "not reachable in our usage because this is TLS client use only against trusted AWS endpoints, does not parse CRLs, and is not exposed to untrusted HTTP/2 peers" — which drops the maintainers' explicit "users who enable it remain affected" caveat.
That's also the entire reason the PR is currently unmergeable. git merge-tree main da7ac62 reports exactly one conflict:
CONFLICT (content): Merge conflict in .cargo/audit.toml
and nothing else — pingora-proxy/src/lib.rs merges cleanly. So rebasing and dropping the second commit resolves the conflict and keeps main's wording. Worth being deliberate about it, since resolving this conflict "in the PR's favour" during a routine rebase is how the weaker note would slip in.
Shard count is sized from the wrong number
ShardedNotify::new() uses available_parallelism(), but pingora's worker count is explicitly configured — pingora-core/src/server/mod.rs:727:
let threads = wrapper.service.threads().unwrap_or(conf.threads);and HttpProxy::new() already has the ServerConf in hand. The two numbers are unrelated, so the sharding can silently under-provision. Measured on a 12-core host (so 16 shards), counting distinct shards reached by N threads:
available_parallelism = Ok(12)
shards allocated = 16
4 threads -> 4 distinct shards
8 threads -> 8 distinct shards
16 threads -> 16 distinct shards
32 threads -> 16 distinct shards
64 threads -> 16 distinct shards
Distribution is perfect up to the shard count and then collapses. The case I'd worry about is the one this PR is aimed at: a container with a restricted CPU quota or affinity mask but an explicitly configured higher threads, where you get few shards and many workers and most of the contention returns. threads is also per-service and not shared, so a process running several services has more worker threads than any single ShardedNotify has shards.
Sizing from conf.threads / service.threads() instead would make the shard count track the thing that actually contends. Non-blocking, but it seems worth doing given it's the whole point of the change.
What I did not test
No 128-core machine here, so I can't reproduce #844's ~65% off-CPU futex measurement or confirm the improvement at that scale — everything above is correctness, not the performance claim. I also didn't run cargo audit itself (not installed here); the redundancy finding is from the git history and the merge, not from running the tool.
Fixes #844.
Every request that parks in
read_request()registers a shutdown waiter on a singleNotifyshared by the whole proxy, and unregisters it when the read completes. Both operations serialize on theNotify's internal mutex, which becomes the scaling bottleneck on many-core machines: #844 measured ~65% of off-CPU time in futex waits on a 128-core NUMA host.This change shards the
Notifyby worker thread (cache-line padded, power-of-two count fromavailable_parallelism, capped at 256). Waiter registration stays on a core-local shard;http_cleanup()notifies all shards. Streams whose headers are already buffered (h2 streams, pipelined h1) never touch theNotifyat all because the biased select pollsread_request()first.It also closes a pre-existing lost-wakeup race:
handle_new_request()never checkedshutdown_flag, so anotify_waiters()that fired betweenread_request()returning Pending and the waiter registering was missed, and the connection lingered until the grace period expired. The shutdown arm now registers the waiter first (Notified::enable), then checksshutdown_flagbefore awaiting. The two lock the same shard mutex, so either the flag load sees the store or the waiter receives the notification.Semantics are otherwise unchanged: connections parked in
read_request()are still woken immediately on shutdown, and in-flight requests are never aborted.Two tests added:
shutdown_wakes_parked_read_requests: parked keep-alive reads across multiple worker threads all wake onhttp_cleanup(). This is the behavior the flag-only fix proposed inNotify-based shutdown inHttpProxycauses severe lock contention on multi-core / NUMA systems #844 would break.shutdown_before_read_request_parks_returns_immediately: regression test for the lost-wakeup race. It hangs on current main.