hanzhe-one commented on issue #7828:
URL: https://github.com/apache/shenyu/issues/7828#issuecomment-6259344716
## Update: the cited implementation has been replaced
Before starting work I re-checked the current `master`, and the code this
report was written against is no longer there.
- PR #7745 ("prevent expired memory cache entries from accumulating") merged
into `master` at 2026-10-19T01:29:11Z (merge commit
`6dc19834f2135365e476bffdafa2c44bda5f44be`).
- Both revisions cited in this report predate that merge: `04f7c2e8b`
(working revision) and `7cc1818ac` (upstream `master`) are both ancestors of
`6dc1983`, verified with `git merge-base --is-ancestor`.
- This issue was created 2026-10-18T14:06Z and claimed 2026-10-19T02:04Z,
i.e. after the merge landed.
`MemoryCache` on current `master` (`81a85f3a4`) is now a single Caffeine
cache with `getIfPresent` and `expireVariably`:
https://github.com/apache/shenyu/blob/81a85f3a4ab3cf799930ae8b1f185f5b4fd72b10/shenyu-plugin/shenyu-plugin-cache/shenyu-plugin-cache-memory/src/main/java/org/apache/shenyu/plugin/cache/memory/MemoryCache.java
## Effect on the task list
- **"Consolidate the read path to avoid existence-check-then-read duplicate
lookups"** — no longer applicable. `getData()` is a single `getIfPresent`
lookup, and `isExist()` has no production callers in `shenyu-plugin-cache` on
`master` (only the interface and the two implementations reference it; the
handler calls `getData()` directly).
- **"Remove unnecessary bounded-elastic scheduling"** — still valid.
`cacheData` (L74), `isExist` (L85) and `getData` (L96) still wrap a local map
operation in `Mono.fromCallable(...).subscribeOn(Schedulers.boundedElastic())`.
- The remaining scope is therefore the scheduling hop on the read/write
path, not lookup consolidation.
## One caveat the report does not cover
`CachePlugin.executeCache` chains
`cache.getData(responseKey).map(CachedResponse::decode)`, and
`CachedResponse.decode` parses the header JSON and copies the body bytes
(`Arrays.copyOfRange`). Those operations currently execute on the
bounded-elastic thread selected by `MemoryCache.getData`. Removing
`subscribeOn` moves that decode/copy work onto the subscribing thread, which
for the gateway is the Netty event loop. That may be a regression for large
response bodies, so the fix is a trade-off rather than a pure win.
The write path also schedules independently: `CachePlugin.cacheResponse`
applies its own `subscribeOn(Schedulers.boundedElastic())` (L216-L218), which
is backend-agnostic and therefore outside a `MemoryCache`-only change.
## Proposed scope
1. Remove the unconditional `subscribeOn(Schedulers.boundedElastic())` from
`MemoryCache.cacheData` / `isExist` / `getData`, keeping the lookups lazy at
subscription time.
2. Add a test that subscribes from a named caller scheduler and asserts the
lookup does not run on a bounded-elastic thread.
3. Document that `CachedResponse.decode` now runs on the caller thread; do
not claim a measured throughput gain — no JMH benchmark exists in this
repository and none was run.
I will leave the write-path scheduling in `CachePlugin` and the Redis
backend out of scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
