Aias00 commented on code in PR #7157:
URL: https://github.com/apache/shenyu/pull/7157#discussion_r4068604841
##########
shenyu-infra/shenyu-infra-redis/src/main/java/org/apache/shenyu/infra/redis/RedisConnectionFactory.java:
##########
@@ -56,6 +62,34 @@ public LettuceConnectionFactory
getLettuceConnectionFactory() {
return this.lettuceConnectionFactory;
}
+ /**
+ * Destroy the lettuce connection factory and the connection pool it owns.
The client this factory
+ * was built for must not be used afterwards.
+ */
+ @Override
+ public void destroy() {
Review Comment:
Adding the lifecycle end here is the right move: this wrapper is the only
type that owns the two things `LettuceConnectionFactory` created but nobody
released - the connection pool and the per-client Netty resources - so
`destroy()` belongs to whoever created them.
I specifically checked the failure mode that would have made this dangerous:
could releasing one Shenyu redis client take down another plugin's client
because they share Lettuce `ClientResources`? It cannot, and the reasoning is
worth writing down because it is load-bearing for every call site below:
1. `getLettuceClientConfiguration(...)` builds
`LettucePoolingClientConfiguration.builder().poolConfig(...).build()` and never
calls `.clientResources(...)`, so the configuration carries no resources.
2. Lettuce 6.3.2, `AbstractRedisClient` constructor (decompiled from
`lettuce-core-6.3.2.RELEASE.jar`): `if (clientResources == null) {
sharedResources = false; clientResources = DefaultClientResources.create(); }`
- each client creates and therefore owns its own resources.
3. `AbstractRedisClient.closeClientResources(...)` only does the full
`clientResources.shutdown(...)` when `sharedResources == false`; otherwise it
just releases the event loop groups it borrowed.
So `destroy()` releases exactly one client's own resources and nothing else.
Two further properties make it safe to call from the handlers:
`LettuceConnectionFactory.stop()` only does work under
`state.compareAndSet(STARTED, STOPPING)`, so a second `destroy()` is a no-op,
and `isRunning()` is a clean observable to assert on.
One note rather than a request: the class is not a Spring bean anywhere, and
`RedisConnectionFactory` is not a `ReactiveRedisConnectionFactory` either, so
`destroyQuietly` can never actually be handed one - every call site passes the
inner `LettuceConnectionFactory`, which has always implemented `DisposableBean`
on its own. Right now the instance `destroy()` has exactly one caller, its own
unit test. It is still the right home for the lifecycle, but if you want it to
earn its keep, the follow-up would be to have the call sites register
themselves here instead of extracting the lettuce factory.
##########
shenyu-plugin/shenyu-plugin-fault-tolerance/shenyu-plugin-ratelimiter/src/main/java/org/apache/shenyu/plugin/ratelimiter/handler/RateLimiterPluginDataHandler.java:
##########
@@ -55,12 +55,17 @@ public void handlerPlugin(final PluginData pluginData) {
if (Objects.isNull(Singleton.INST.get(ReactiveRedisTemplate.class))
||
Objects.isNull(Singleton.INST.get(RedisConfigProperties.class))
||
!redisConfigProperties.equals(Singleton.INST.get(RedisConfigProperties.class)))
{
+ final ReactiveRedisTemplate previousRedisTemplate =
Singleton.INST.get(ReactiveRedisTemplate.class);
Review Comment:
Since `Singleton.INST` is keyed on `ReactiveRedisTemplate.class`, it is
worth being explicit about what this entry is: a process-wide slot, not
per-plugin state. I went looking for a second writer and there isn't one -
`RateLimiterPluginDataHandler` is the only place that calls
`Singleton.INST.single(ReactiveRedisTemplate.class, ...)`; `RedisRateLimiter`
and `ConcurrentRateLimiterAlgorithm` only read it. So destroying what was there
before is safe: whatever it was, this handler put it there.
Two smaller observations:
- Caching `previousRedisTemplate` *before* the new one is installed is
correct. What it does not close is the gap between a request reading
`Singleton.INST.get(ReactiveRedisTemplate.class)` and the moment it subscribes
- a config push landing inside that window now fails the request where
previously it would have succeeded (the old client stayed usable). That is the
tradeoff documented in the PR description, and I agree it is worth paying to
stop leaking a pool per config change; just calling it out so it is a recorded
decision rather than an accident. If you want to narrow it later, the fix
belongs in a follow-up.
- Note the `||` chain above means an entry can exist with
`RedisConfigProperties.class` absent, so rebuild also covers the legacy
half-initialised state - good, and the new destroy handles it too.
##########
shenyu-plugin/shenyu-plugin-cache/shenyu-plugin-cache-redis/src/main/java/org/apache/shenyu/plugin/cache/redis/RedisCache.java:
##########
@@ -89,5 +89,7 @@ public void close() {
connection.close();
} catch (Exception ignored) {
}
+ // the factory owns the connection pool and its threads, closing a
connection does not release them
+ RedisConnectionFactory.destroyQuietly(connectionFactory);
Review Comment:
This is the right hook to release on. Worth being aware of how the caller
sequences it, because this one is looser than the other three sites in this PR:
`CachePluginDataHandler`:
```java
this.closeCacheIfNeed(); //
destroys the old cache here
final ICacheBuilder cacheBuilder = ExtensionLoader...getJoin(...);
Singleton.INST.single(ICache.class, cacheBuilder.builderCache(config)); //
new one installed later
```
The old hand - `closeCacheIfNeed()` - only does this:
```java
ICache lastCache = CacheUtils.getCache(); //
Singleton.INST.get(ICache.class)
...
lastCache.close();
```
It never removes `ICache.class` from `Singleton.INST`. So between the
destroy above and the install below, `CacheUtils.getCache()` still hands out
the destroyed cache, and after `removePlugin` it stays there indefinitely.
Before this change that window was survivable: `close()` only returned two
borrowed connections to the pool, so a request that had already grabbed the
cache still worked. Now the factory underneath it is destroyed, so that same
request fails. Same class of window the other handlers have, but here it can be
closed cheaply:
- either install first, then close the previous one (what you did for the
other three handlers), or
- have `closeCacheIfNeed()` evict `ICache.class` right after
`lastCache.close()`.
Also happy to leave it as a follow-up issue if you prefer to keep this PR
scoped to the leak - just say so and I will drop it. Flagging it because the
cache plugin is the one replacement path where the old instance stays reachable
after being destroyed.
Detail, no action needed: with pooling enabled,
`connectionFactory.getReactiveConnection()` borrows a pooled connection and
`close()` returns it, so the two lines above behave as intended rather than
opening throwaway sockets.
Note on verification: I could not execute this one. `RedisCacheTest` is
compiled and declares three `@Test` methods, but Surefire reports `Tests run:
0` for it in this module (targeted and untargeted runs alike) on my machine -
same before your change, so it is not something this PR introduced. I checked
the existing `closeCache()` test by reading: its last `close()` runs against
mocked factories, which are not `DisposableBean`, so `destroyQuietly` no-ops
there, and the real cache is not used after `close()`.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-sensitive-word/src/main/java/org/apache/shenyu/plugin/ai/sensitive/word/handler/SensitiveWordPluginDataHandler.java:
##########
@@ -89,19 +89,28 @@ public void handlerPlugin(final PluginData pluginData) {
return;
}
RedisConfigProperties cachedProperties =
REDIS_PROPERTIES.get().obtainHandle(PLUGIN_NAME);
- if (Objects.isNull(REDIS_TEMPLATES.get().obtainHandle(PLUGIN_NAME)) ||
!redisConfig.equals(cachedProperties)) {
+ ReactiveRedisTemplate<String, String> cachedTemplate =
REDIS_TEMPLATES.get().obtainHandle(PLUGIN_NAME);
+ if (Objects.isNull(cachedTemplate) ||
!redisConfig.equals(cachedProperties)) {
RedisConnectionFactory connectionFactory = new
RedisConnectionFactory(redisConfig);
ReactiveRedisTemplate<String, String> redisTemplate = new
ShenyuReactiveRedisTemplate<>(
connectionFactory.getLettuceConnectionFactory(),
ShenyuRedisSerializationContext.stringSerializationContext());
REDIS_TEMPLATES.get().cachedHandle(PLUGIN_NAME, redisTemplate);
REDIS_PROPERTIES.get().cachedHandle(PLUGIN_NAME, redisConfig);
+ // the client that is replaced must not keep its connection pool
and its threads alive
+ if (Objects.nonNull(cachedTemplate)) {
+
RedisConnectionFactory.destroyQuietly(cachedTemplate.getConnectionFactory());
Review Comment:
Installing the new template with `cachedHandle(...)` *before* destroying the
old one is exactly the right order - new requests can never observe a destroyed
client. Capturing `cachedTemplate` before mutating the caches is also
necessary, since after `cachedHandle` the handle is already the new template.
Same thing in `removePlugin` just below, where destroying before `removeHandle`
keeps the DTO reachable for the release.
Two things you may want to fold in later, neither blocking:
1. With pooling configured (`LettucePoolingClientConfiguration`), what was
actually being leaked here per config change was the pool plus one owned set of
Netty event loops - see the comment on `RedisConnectionFactory#destroy` for why
releasing it does not disturb the other plugins' clients. Would be nice to have
a note here saying the released client must not be reused, e.g. by using it
from a `finally` that swaps the reference.
2. `removePlugin` now releases the client, which this handler did not do
before - good. For symmetry, `AiTokenLimiterPluginHandler` and
`RateLimiterPluginDataHandler` still have no `removePlugin`: those clients stay
alive (with their pool and threads) for the rest of the process lifetime after
the plugin is removed, because nothing ever clears their `CommonHandleCache` /
`Singleton` entries. Not a regression introduced here, but it is the same bug
through a different door, and now there is a `destroyQuietly` helper that makes
the follow-up a two-line change per handler.
--
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]
