HY-love-sleep opened a new issue, #7163:
URL: https://github.com/apache/shenyu/issues/7163
### Context
Follow-ups raised in the review of #7157, which added the lifecycle end to
`RedisConnectionFactory` and
made the four sites that replace a redis client release the one they
replace. All three items below are
the same story from different angles: a client that is replaced or removed
stays reachable, or is never
released at all.
### 1. `CachePluginDataHandler`: the previous cache is destroyed before the
new one is installed
`handlerPlugin(...)` currently does:
```java
Singleton.INST.single(CacheConfig.class, cacheConfig);
this.closeCacheIfNeed(); //
destroys the previous cache here
final ICacheBuilder cacheBuilder = ExtensionLoader...getJoin(...);
Singleton.INST.single(ICache.class, cacheBuilder.builderCache(config)); //
installs the new one later
```
and `closeCacheIfNeed()` only closes the previous cache — it never clears
`ICache.class` from
`Singleton`. So between those two statements `CacheUtils.getCache()` still
hands out the cache whose
connection factory was just destroyed, and a request that lands in that
window fails. Before #7157 the
window was survivable, because `RedisCache.close()` only returned borrowed
connections to the pool.
Fix: install the new cache first, then close the previous one — the order
the three handlers changed by
#7157 already use. Evicting `ICache.class` instead is not possible today:
`Singleton` has no removal API,
and the readers of `CacheUtils.getCache()` do not null-check it.
### 2. `AiTokenLimiterPluginHandler` and `RateLimiterPluginDataHandler` have
no `removePlugin`
Both keep their client in a process-wide place — a `CommonHandleCache` and
`Singleton.INST` respectively —
so when the plugin is removed or disabled, the client, its connection pool
and its Netty event loops stay
alive for the rest of the process lifetime. `SensitiveWordPluginDataHandler`
releases its client in
`removePlugin` (added by #7157), and `destroyQuietly(...)` makes this a
couple of lines per handler.
- **`AiTokenLimiterPluginHandler`** is straightforward: destroy the cached
client and clear the two caches.
- **`RateLimiterPluginDataHandler`** needs an eviction point first.
`Singleton` has no removal API, so
destroying the client in `removePlugin` while leaving the entry behind
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)`. Either `Singleton`
gains a removal API
(`remove(Class)`), or this handler keeps its client in a
`CommonHandleCache` like the others.
### 3. Note only: `RedisConnectionFactory.destroy()` has no production
caller yet
The instance `destroy()` belongs to the wrapper that owns the pool, but
every call site holds the reactive
template, so it passes the inner `LettuceConnectionFactory` to
`destroyQuietly(...)` — which is enough,
the lettuce factory has implemented `DisposableBean` all along. For the
wrapper to "earn its keep", the
call sites would have to keep the `RedisConnectionFactory` instead of
extracting the lettuce factory from
it. No behaviour change involved; recorded for completeness.
### Plan
I am happy to take 1 and the `AiTokenLimiterPluginHandler` half of 2 in a
follow-up PR. For the rate
limiter I would rather hear which eviction point you prefer before writing
it, @Aias00 — adding a
removal API to `Singleton` (used across plugins) is a bigger decision than
the fix itself.
--
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]
