Repository navigation
feat(cache): coalesce concurrent cache misses into one upstream fill - #7969
Conversation
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
left a comment
There was a problem hiding this comment.
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.buildSingleFlightreturnsResponseSingleFlight.disabled()unlesssingleFlightEnabledis set, andCacheUtils.getSingleFlight()is null-safe so an unconfigured gateway behaves exactly as before.testDisabledCoalescingForwardsEveryRequestpins that (2upstream calls, follower gets its own body). - Followers never touch the leader's buffers. The shared unit is
byte[]produced byCachedResponse.encode(), and each follower runsCachedResponse.decodeon its own array before writing to its own response.testConcurrentMissesShareOneFillasserts 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 withNettyDataBufferreference counting. - Reuse is bounded by what the cache itself would have stored.
CachedResponse.capturealready refusesstatus < 200,206,304,Set-Cookie,Vary,no-store/no-cache/private, and hop-by-hop headers;complete()then appliesmaxResponseByteson top. So coalescing can never merge two responses the cache would have kept apart.testFollowerFillsWhenLeaderResponseCannotBeSharedexercises theSet-Cookiebranch 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 inorElseGet(() -> 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())plusFill.finishedCAS 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.testCancelledFollowerReleasesItsSlotcovers the waiter-counter side from the other direction. publish()publishes beforefills.remove(...). A request arriving in the window betweentryEmitValueandremovestill 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, andmaxResponseBytes <= 0means "share nothing" rather than "no limit", which is the safe reading.
Non-blocking
- No test covers
maxKeyscapacity. Whenfills.size() >= maxKeys,lead()returns empty and the request fills itself. That is the one branch oflead()with no coverage (themaxWaitersanalogue is covered). A case withmaxKeys = 1, two distinct keys, asserting two upstream calls would close it. - No test covers a failing leader.
testFollowerFillsWhenLeaderResponseCannotBeSharedcovers "leader completed but produced nothing shareable"; it does not cover "leader's chain errored", where the follower is resolved byrelease()rather than bycomplete(empty). Both reach the same code, but the error path is the one that also has to surviveonErrorResumein the plugin chain, so it is worth pinning separately. - Follower status replay is untested. Every case writes a 200 body, so
CachedResponse.writeTo'ssetStatusCode(...)is never exercised through the follower path. A leader returning201(or404, which is cacheable here sincecaptureonly rejects< 200) with the follower asserting the same status would prove the metadata leg travels, not just the body. singleFlightWaitTimeoutMillis = 1000may 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.expireStale()only runs fromlead()when at capacity. A fill stuck on an idle key therefore lingers until the next capacity event. It is bounded bymaxKeysanddoFinallycovers the normal error/cancel paths, so this is not a leak — but a periodic sweep (or anexpireStale()call infollow()) would make the bound time-based rather than traffic-based.fills.size() >= maxKeysfollowed byputIfAbsentis a soft limit. Concurrent leaders can transiently exceedmaxKeys. Benign for a capacity guard; worth a comment so nobody later reads it as a hard invariant.handlerPluginswaps the coordinator on every config reload viaSingleton.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 inbuildSingleFlight's Javadoc would settle it.- This is a new operator-facing feature with five knobs;
RELEASE-NOTES.mdhas no entry. The repo has a### New Featuressection that otherfeat:PRs use, and thecacheplugin 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
left a comment
There was a problem hiding this comment.
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.handlerPluginshort-circuits oncacheConfig.equals(lastCacheConfig), andCacheConfig.equalscomparescacheTypeand the rawconfigstring. Any single-flight knob change therefore changes the raw JSON, so the coordinator is rebuilt - no stale-config trap.closeCacheIfNeedresetting the singleton todisabled()is the right shutdown behaviour. - Fallbacks never widen eligibility.
lead()empty (capacity / already filling) ->follow()empty (no fill or waiter cap) -> plainforward(). A follower that times out, seesOptional.empty()from the leader, exceedsmaxResponseBytes, 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 callsCachedResponse.decodeon its own copy; noDataBufferis shared across exchanges, and the leader keeps its own buffer. Correct. - Release is idempotent.
Fill.finishedCAS makes thedoFinallyrelease a no-op after a successfulcomplete, so a leader that published and then errored cannot release the key twice. - Capacity.
lead()documents thatsize()plusputIfAbsentis not atomic and can overshootmaxKeysby the number of racing requests. Honest, and bounded.
Two non-blocking observations:
expireStale()runs an O(active fills) stream on everyfollow(), i.e. on every cache miss while a fill is in flight. At the defaultmaxKeys = 1024that is cheap, but it is on the request path; a time-gated sweep (e.g. at most once perfillTimeout / 4) would keep the worst case off the hot path.singleFlightWaitTimeoutMillis = 1000withfillTimeoutMillis = 30000means 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.
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
Vary,private/no-store/no-cache, 206, 304, non-2xx)singleFlightWaitTimeoutMillissingleFlightMaxResponseBytesA 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:
singleFlightEnabledfalsesingleFlightMaxKeys1024singleFlightMaxWaiters64singleFlightWaitTimeoutMillis1000singleFlightFillTimeoutMillis30000singleFlightMaxResponseBytes1048576Coordination is per gateway instance. Replicas can still fill the same key independently; no distributed lock is taken.
Tests
CacheSingleFlightTestcovers coalescing, the disabled path, each fallback above, distinct keys, and follower cancellation. The existing cache suite is unchanged and passes.Not in this PR
CacheRuleHandleinshenyu-common.ApplicationConfigCacheinvalidation.Verification
mvn test -pl shenyu-plugin/shenyu-plugin-cache/shenyu-plugin-cache-handler— 105 tests pass. Checkstyle and RAT pass.Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.