Skip to content

feat(cache): coalesce concurrent cache misses into one upstream fill - #7969

Merged
Aias00 merged 2 commits into
apache:masterfrom
hanzhe-one:feat/cache-single-flight
Oct 23, 2026
Merged

Aias00 merged 2 commits into
apache:masterfrom
hanzhe-one:feat/cache-single-flight

Conversation

@hanzhe-one

Copy link
Copy Markdown
Contributor

Fixes #7932 (first stage).

Concurrent requests that miss the same cache key each forward to the upstream, so a burst on one cold key produces one upstream call per request. This adds a per-gateway coordinator so the first request fills the key and later requests on the same key replay the captured response.

Behaviour

Case Result
Concurrent misses on one eligible key One upstream fill; followers replay its bytes
Leader response is not reusable (cookie, Vary, private/no-store/no-cache, 206, 304, non-2xx) Followers fill the key themselves
Follower waits longer than singleFlightWaitTimeoutMillis Follower fills the key itself
Waiter slots for a key are exhausted Extra requests fill the key themselves
Response larger than singleFlightMaxResponseBytes Not shared; followers fill the key themselves
Leader errors or is cancelled Key released, the next request leads a new fill
Feature disabled Every request forwards, exactly as before

A follower never subscribes to the leader's response publisher and never shares a DataBuffer; it decodes its own copy of the packet into its own response. Keys are the existing response cache keys, so coalescing never widens cache eligibility or merges responses the cache keeps apart.

Configuration

Added to the cache plugin config, off by default:

Field Default
singleFlightEnabled false
singleFlightMaxKeys 1024
singleFlightMaxWaiters 64
singleFlightWaitTimeoutMillis 1000
singleFlightFillTimeoutMillis 30000
singleFlightMaxResponseBytes 1048576

Coordination is per gateway instance. Replicas can still fill the same key independently; no distributed lock is taken.

Tests

CacheSingleFlightTest covers coalescing, the disabled path, each fallback above, distinct keys, and follower cancellation. The existing cache suite is unchanged and passes.

Not in this PR

  • Stale-while-revalidate and separate freshness/stale deadlines. The issue asks for these only after the miss path is correct, and they need new fields on CacheRuleHandle in shenyu-common.
  • Invalidation/configuration generations preventing an old fill from repopulating an invalidated entry. The cache has no generation concept yet; adding it touches ApplicationConfigCache invalidation.
  • Benchmark comparison under [Task][Performance] Establish reproducible benchmarks and track gateway optimizations #7650.

Verification

mvn test -pl shenyu-plugin/shenyu-plugin-cache/shenyu-plugin-cache-handler — 105 tests pass. Checkstyle and RAT pass.


Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

Concurrent requests that miss the same cache key each forward to the
upstream, so a burst on one cold key produces one upstream call per
request. Add a per-gateway coordinator that lets the first request fill
the key and has later requests on the same key replay the captured
response instead.

A follower never subscribes to the leader's response publisher and never
shares a DataBuffer: it decodes its own copy of the response packet into
its own response. When the leader cannot produce a reusable response, or
a follower waits too long, runs out of waiter slots, or would receive a
response above the configured size, the follower fills the key itself,
which is the behaviour without coalescing.

Keys are exactly the existing response cache keys, so coalescing never
widens cache eligibility or merges responses the cache keeps apart. The
feature is off by default and bounded by filling keys, waiters per key,
wait time, fill time and shared response size. Coordination is per
gateway instance; replicas may still fill the same key independently.

Stale-while-revalidate and configuration generations are follow-ups.
Aias00
Aias00 previously approved these changes Oct 22, 2026

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: #7969 feat(cache): coalesce concurrent cache misses into one upstream fill

Verdict: approved — the coalescing design is sound, opt-in, and degrades to the current behaviour on every failure path. Test coverage is unusually good for a concurrency feature: eight cases, each isolating one degradation branch.

Why the shape is right

  • Opt-in with a disabled() no-op. CachePluginDataHandler.buildSingleFlight returns ResponseSingleFlight.disabled() unless singleFlightEnabled is set, and CacheUtils.getSingleFlight() is null-safe so an unconfigured gateway behaves exactly as before. testDisabledCoalescingForwardsEveryRequest pins that (2 upstream calls, follower gets its own body).
  • Followers never touch the leader's buffers. The shared unit is byte[] produced by CachedResponse.encode(), and each follower runs CachedResponse.decode on its own array before writing to its own response. testConcurrentMissesShareOneFill asserts both exchanges end with the body, which is only possible if the second request decoded its own copy. This is the detail that makes the feature safe with NettyDataBuffer reference counting.
  • Reuse is bounded by what the cache itself would have stored. CachedResponse.capture already refuses status < 200, 206, 304, Set-Cookie, Vary, no-store/no-cache/private, and hop-by-hop headers; complete() then applies maxResponseBytes on top. So coalescing can never merge two responses the cache would have kept apart. testFollowerFillsWhenLeaderResponseCannotBeShared exercises the Set-Cookie branch and shows the follower filling instead of receiving private state — that is the test I would have asked for.
  • Every degradation path falls back to "fill it yourself", never to an error. Timeout (testFollowerFillsAfterWaitingTooLong), no waiter slot (testFollowerFillsWhenNoWaiterSlotIsAvailable), oversized response (testFollowerFillsWhenResponseExceedsShareLimit), undecodable packet — all end in orElseGet(() -> forward(...)). That is the correct default: worst case is today's behaviour.
  • The key is released on error and cancellation. forward(...).doFinally(ignored -> fill.release()) plus Fill.finished CAS means a publish and a later release cannot both take effect, so a leader that already published is not disturbed by its own cancellation, and a leader that fails does not leave the key owned. testCancelledFollowerReleasesItsSlot covers the waiter-counter side from the other direction.
  • publish() publishes before fills.remove(...). A request arriving in the window between tryEmitValue and remove still finds the fill and replays the packet rather than refilling. The ordering is deliberate and commented, which matters because reversing those two lines would be invisible in the tests but would let a burst leak an extra upstream call.
  • Config surface is complete and sane. singleFlightMaxKeys=1024, maxWaiters=64, waitTimeout=1000ms, fillTimeout=30000ms, maxResponseBytes=1MiB — each has a documented purpose, and maxResponseBytes <= 0 means "share nothing" rather than "no limit", which is the safe reading.

Non-blocking

  1. No test covers maxKeys capacity. When fills.size() >= maxKeys, lead() returns empty and the request fills itself. That is the one branch of lead() with no coverage (the maxWaiters analogue is covered). A case with maxKeys = 1, two distinct keys, asserting two upstream calls would close it.
  2. No test covers a failing leader. testFollowerFillsWhenLeaderResponseCannotBeShared covers "leader completed but produced nothing shareable"; it does not cover "leader's chain errored", where the follower is resolved by release() rather than by complete(empty). Both reach the same code, but the error path is the one that also has to survive onErrorResume in the plugin chain, so it is worth pinning separately.
  3. Follower status replay is untested. Every case writes a 200 body, so CachedResponse.writeTo's setStatusCode(...) is never exercised through the follower path. A leader returning 201 (or 404, which is cacheable here since capture only rejects < 200) with the follower asserting the same status would prove the metadata leg travels, not just the body.
  4. singleFlightWaitTimeoutMillis = 1000 may partly defeat the feature. A follower that gives up after 1s re-fires the upstream call, so for upstreams slower than a second the coalescing window closes before the leader finishes. That default is defensible (it bounds latency), but the trade-off deserves a line in the docs: the setting should be at least the upstream p99 if the goal is to actually collapse bursts.
  5. expireStale() only runs from lead() when at capacity. A fill stuck on an idle key therefore lingers until the next capacity event. It is bounded by maxKeys and doFinally covers the normal error/cancel paths, so this is not a leak — but a periodic sweep (or an expireStale() call in follow()) would make the bound time-based rather than traffic-based.
  6. fills.size() >= maxKeys followed by putIfAbsent is a soft limit. Concurrent leaders can transiently exceed maxKeys. Benign for a capacity guard; worth a comment so nobody later reads it as a hard invariant.
  7. handlerPlugin swaps the coordinator on every config reload via Singleton.INST.single(...). In-flight fills keep running on the orphaned instance and their waiters are still signalled, so nothing hangs; a follower arriving immediately after the swap simply starts a new fill on the fresh instance. Acceptable, but a reader will wonder — one sentence in buildSingleFlight's Javadoc would settle it.
  8. This is a new operator-facing feature with five knobs; RELEASE-NOTES.md has no entry. The repo has a ### New Features section that other feat: PRs use, and the cache plugin docs page should mention that coalescing changes the upstream call pattern from N per burst to 1 per burst (per gateway instance — the class Javadoc is explicit that replicas still fill independently, which is the right scope but needs to reach the docs too).

One thing I want to be explicit about

Followers write the replayed response straight to exchange.getResponse(), bypassing the rest of the chain. That is consistent with the existing cache-hit path (dataFlg.get().writeTo(exchange.getResponse())), so post-cache plugins already do not see cache hits — but it does mean coalesced responses behave like cache hits rather than like fresh upstream responses. Anyone debugging "why did plugin X not run for the 5th concurrent request" will land here, so it belongs in the plugin docs alongside the feature description.

CI is still queued on this head.

Add the three branches the review called out as uncovered:

- `maxKeys` capacity: with one key slot taken, a second key forwards
  instead of being folded into another fill.
- A leader whose chain fails, where the follower is resolved by the
  release path rather than by a published packet.
- Follower status replay: a 201 and a 404 leader both reach the follower
  with their status and headers, not just the body.

Also sweep expired fills from `follow()`, so a fill whose leader vanished
on an idle key is released by time instead of waiting for the next
request to reach the key limit, and note the soft nature of the `maxKeys`
check and the coordinator swap on config reload.

Add the operator-facing RELEASE-NOTES entry under Unreleased.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review after de910cb6: still good, now fully covered

Since my previous approval the PR grew a RELEASE-NOTES.md entry, the CacheConfig knobs (singleFlightEnabled, maxKeys, maxWaiters, waitTimeoutMillis, fillTimeoutMillis, maxResponseBytes) and CacheSingleFlightTest. Re-checked the parts that matter:

  • Configuration reload. CachePluginDataHandler.handlerPlugin short-circuits on cacheConfig.equals(lastCacheConfig), and CacheConfig.equals compares cacheType and the raw config string. Any single-flight knob change therefore changes the raw JSON, so the coordinator is rebuilt - no stale-config trap. closeCacheIfNeed resetting the singleton to disabled() is the right shutdown behaviour.
  • Fallbacks never widen eligibility. lead() empty (capacity / already filling) -> follow() empty (no fill or waiter cap) -> plain forward(). A follower that times out, sees Optional.empty() from the leader, exceeds maxResponseBytes, or cannot decode the packet forwards itself. Every branch ends in "behave as before", which is the property to preserve.
  • Byte ownership. Followers only ever receive a byte[] and each calls CachedResponse.decode on its own copy; no DataBuffer is shared across exchanges, and the leader keeps its own buffer. Correct.
  • Release is idempotent. Fill.finished CAS makes the doFinally release a no-op after a successful complete, so a leader that published and then errored cannot release the key twice.
  • Capacity. lead() documents that size() plus putIfAbsent is not atomic and can overshoot maxKeys by the number of racing requests. Honest, and bounded.

Two non-blocking observations:

  1. expireStale() runs an O(active fills) stream on every follow(), i.e. on every cache miss while a fill is in flight. At the default maxKeys = 1024 that is cheap, but it is on the request path; a time-gated sweep (e.g. at most once per fillTimeout / 4) would keep the worst case off the hot path.
  2. singleFlightWaitTimeoutMillis = 1000 with fillTimeoutMillis = 30000 means a client waits at most 1s and then forwards anyway. That is the documented behaviour, and the release note tells operators to raise the wait timeout above upstream p99 - good, but it is the one setting that makes the feature look like it "does not work" when the upstream is slower than 1s.

@Aias00
Aias00 merged commit 9696a3a into apache:master Oct 23, 2026
28 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.

[Enhancement][Cache] Coalesce concurrent cache misses and support bounded stale refresh

2 participants