HY-love-sleep opened a new pull request, #7164:
URL: https://github.com/apache/shenyu/pull/7164
### Motivation
Follow-up of #7157, tracked by #7163. Two of its three items are fixed here;
the third one needs a
decision first, see *Not in this PR* below.
### 1. `CachePluginDataHandler`: the new cache is installed before the
previous one is closed
`handlerPlugin(...)` closed the previous cache first (`closeCacheIfNeed()`)
and installed the new one
afterwards, while the `Singleton` entry keeps pointing at the old instance
until that install. Between the
two statements `CacheUtils.getCache()` therefore handed out the cache whose
connection factory had just
been released by #7157, so a request landing in that window failed. The new
cache is now built and
installed first, and the previous one is closed after it — the order the
three handlers changed by #7157
already use. `closeCacheIfNeed()` keeps its meaning for the disabled and
removed paths, where no request
can reach the cache any more.
Evicting `ICache.class` instead (the other option raised in the review) is
not possible today: `Singleton`
has no removal API, and the readers of `CacheUtils.getCache()` do not
null-check it.
### 2. `AiTokenLimiterPluginHandler` releases its client in `removePlugin`
The handler keeps its client in a `CommonHandleCache`, so removing or
disabling the plugin left the client,
its connection pool and its Netty event loops alive for the rest of the
process. `removePlugin` now
destroys it and clears both caches, like `SensitiveWordPluginDataHandler`
does since #7157.
### Not in this PR
`RateLimiterPluginDataHandler` has the same gap, but its client lives in
`Singleton.INST` and `Singleton`
has no removal API. Destroying it without evicting the entry would make the
next `handlerPlugin` treat the
destroyed client as valid — it rebuilds only when `RedisConfigProperties`
differs — and hand it to
`RedisRateLimiter` / `ConcurrentRateLimiterAlgorithm`, which do not
null-check
`Singleton.INST.get(ReactiveRedisTemplate.class)`. The eviction point has to
be decided first, so I asked in
#7163 whether `Singleton` should grow a `remove(Class)` or this handler
should keep its client in a
`CommonHandleCache` like the others.
### Testing
```
./mvnw -pl shenyu-plugin/shenyu-plugin-cache/shenyu-plugin-cache-handler,\
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-token-limiter -am test
```
`BUILD SUCCESS`, checkstyle clean:
- `CachePluginDataHandlerReplacementTest` 1/1 — a new class that captures,
from a `close()` callback, which
cache `CacheUtils.getCache()` hands out while the previous one is being
closed, and asserts that it is
already the new one. It is a separate class on purpose:
`CachePluginDataHandlerTest` starts an embedded
redis in `@BeforeAll` and reports `Tests run: 0` on arm64 macOS (also
without this change), so a test
added there would not have run.
- `AiTokenLimiterPluginHandlerTest` 5/5, including the new `removePlugin`
case: the released client stops
running and both caches are cleared.
--
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]
