Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
import com.google.common.collect.Lists;
import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
import org.apache.shenyu.common.enums.RedisModeEnum;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.DisposableBean;
import org.springframework.data.redis.connection.ReactiveRedisConnectionFactory;
import org.springframework.data.redis.connection.RedisClusterConfiguration;
import org.springframework.data.redis.connection.RedisNode;
import org.springframework.data.redis.connection.RedisPassword;
Expand All @@ -38,7 +42,9 @@
/**
* RedisConnectionFactory.
*/
public class RedisConnectionFactory {
public class RedisConnectionFactory implements DisposableBean {

private static final Logger LOG = LoggerFactory.getLogger(RedisConnectionFactory.class);

private final LettuceConnectionFactory lettuceConnectionFactory;

Expand All @@ -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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lettuceConnectionFactory.destroy();
}

/**
* Destroy a connection factory that was created by this class or by {@link #getLettuceConnectionFactory()}.
* The handlers of the plugins that rebuild their client on a configuration change keep the reactive
* template only, so this is how they release the client they replace. A null factory, or one that has
* no lifecycle, is ignored; a failure is logged rather than thrown, because a client that cannot be
* released must not break the configuration update that replaces it.
*
* @param connectionFactory the connection factory to destroy, may be null
*/
public static void destroyQuietly(final ReactiveRedisConnectionFactory connectionFactory) {
if (connectionFactory instanceof DisposableBean) {
try {
((DisposableBean) connectionFactory).destroy();
} catch (Exception e) {
LOG.warn("failed to destroy the redis connection factory", e);
}
}
}

private LettuceConnectionFactory createLettuceConnectionFactory(final RedisConfigProperties redisConfigProperties) {
LettuceClientConfiguration lettuceClientConfiguration = getLettuceClientConfiguration(redisConfigProperties);
if (RedisModeEnum.SENTINEL.getName().equals(redisConfigProperties.getMode())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
import org.springframework.beans.factory.DisposableBean;
import org.springframework.data.redis.connection.ReactiveRedisConnectionFactory;
import org.springframework.data.redis.connection.RedisNode;
import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory;

import java.time.Duration;
import java.lang.reflect.Method;
Expand Down Expand Up @@ -53,6 +56,38 @@ public void redisConnectionFactoryTest() {
Assertions.assertDoesNotThrow(() -> new RedisConnectionFactory(redisConfigProperties));
}

@Test
public void destroyDestroysTheLettuceFactory() {
RedisConfigProperties redisConfigProperties = new RedisConfigProperties();
redisConfigProperties.setUrl("localhost:6379");
redisConfigProperties.setMode(RedisModeEnum.STANDALONE.getName());
RedisConnectionFactory factory = new RedisConnectionFactory(redisConfigProperties);
LettuceConnectionFactory lettuceConnectionFactory = factory.getLettuceConnectionFactory();
Assertions.assertTrue(lettuceConnectionFactory.isRunning());
factory.destroy();
Assertions.assertFalse(lettuceConnectionFactory.isRunning());
}

@Test
public void destroyQuietlyDestroysTheFactory() throws Exception {
DisposableReactiveFactory connectionFactory = Mockito.mock(DisposableReactiveFactory.class);
RedisConnectionFactory.destroyQuietly(connectionFactory);
Mockito.verify(connectionFactory).destroy();
}

@Test
public void destroyQuietlyIgnoresWhatItCannotDestroy() throws Exception {
// nothing to destroy
Assertions.assertDoesNotThrow(() -> RedisConnectionFactory.destroyQuietly(null));
// a factory of another type, without a lifecycle, is left alone
Assertions.assertDoesNotThrow(() -> RedisConnectionFactory.destroyQuietly(
Mockito.mock(ReactiveRedisConnectionFactory.class)));
// a failure is logged instead of thrown: replacing a client must not fail because of it
DisposableReactiveFactory failing = Mockito.mock(DisposableReactiveFactory.class);
Mockito.doThrow(new IllegalStateException("boom")).when(failing).destroy();
Assertions.assertDoesNotThrow(() -> RedisConnectionFactory.destroyQuietly(failing));
}

@Test
public void parseRedisNodeValidInputs() throws Exception {
RedisConnectionFactory factory = createFactoryWithDefaultUrl();
Expand Down Expand Up @@ -118,4 +153,10 @@ private void assertInvalidNode(final Method parseMethod, final RedisConnectionFa
}
});
}

/**
* A reactive factory that has a lifecycle, which is what the lettuce factory is in production.
*/
private interface DisposableReactiveFactory extends ReactiveRedisConnectionFactory, DisposableBean {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
LOG.info("sensitive word plugin: cached the reactive redis template");
}
}

@Override
public void removePlugin(final PluginData pluginData) {
ReactiveRedisTemplate<String, String> cachedTemplate = REDIS_TEMPLATES.get().obtainHandle(PLUGIN_NAME);
if (Objects.nonNull(cachedTemplate)) {
RedisConnectionFactory.destroyQuietly(cachedTemplate.getConnectionFactory());
}
REDIS_TEMPLATES.get().removeHandle(PLUGIN_NAME);
REDIS_PROPERTIES.get().removeHandle(PLUGIN_NAME);
LOG.info("sensitive word plugin: released the cached redis template");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,15 @@
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory;
import org.springframework.data.redis.core.ReactiveRedisTemplate;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;

/**
Expand Down Expand Up @@ -146,6 +151,44 @@ public void testRemovePluginReleasesTheRedisTemplate() {
.obtainHandle(SensitiveWordPluginDataHandler.PLUGIN_NAME));
}

@Test
public void testHandlerPluginDestroysTheClientItReplaces() {
handler.handlerPlugin(pluginData("127.0.0.1:6379"));
ReactiveRedisTemplate<String, String> first = SensitiveWordPluginDataHandler.REDIS_TEMPLATES.get()
.obtainHandle(SensitiveWordPluginDataHandler.PLUGIN_NAME);
assertNotNull(first);
assertTrue(lettuceFactory(first).isRunning());

handler.handlerPlugin(pluginData("127.0.0.1:6380"));
ReactiveRedisTemplate<String, String> second = SensitiveWordPluginDataHandler.REDIS_TEMPLATES.get()
.obtainHandle(SensitiveWordPluginDataHandler.PLUGIN_NAME);
assertNotSame(first, second);
// the client that was replaced must not keep its connection pool and its threads alive
assertFalse(lettuceFactory(first).isRunning());
assertTrue(lettuceFactory(second).isRunning());
}

@Test
public void testHandlerPluginKeepsTheClientWhenTheConfigurationIsUnchanged() {
handler.handlerPlugin(pluginData("127.0.0.1:6379"));
ReactiveRedisTemplate<String, String> first = SensitiveWordPluginDataHandler.REDIS_TEMPLATES.get()
.obtainHandle(SensitiveWordPluginDataHandler.PLUGIN_NAME);
handler.handlerPlugin(pluginData("127.0.0.1:6379"));
assertSame(first, SensitiveWordPluginDataHandler.REDIS_TEMPLATES.get()
.obtainHandle(SensitiveWordPluginDataHandler.PLUGIN_NAME));
assertTrue(lettuceFactory(first).isRunning());
}

@Test
public void testRemovePluginDestroysTheClient() {
handler.handlerPlugin(pluginData("127.0.0.1:6379"));
ReactiveRedisTemplate<String, String> cached = SensitiveWordPluginDataHandler.REDIS_TEMPLATES.get()
.obtainHandle(SensitiveWordPluginDataHandler.PLUGIN_NAME);
assertNotNull(cached);
handler.removePlugin(new PluginData());
assertFalse(lettuceFactory(cached).isRunning());
}

@Test
public void testCachedDictionaryExpires() {
CachedDictionary dictionary = new CachedDictionary(AhoCorasick.empty());
Expand All @@ -154,6 +197,17 @@ public void testCachedDictionaryExpires() {
assertTrue(!dictionary.isExpired(300L));
}

private PluginData pluginData(final String url) {
PluginData pluginData = new PluginData();
pluginData.setEnabled(true);
pluginData.setConfig("{\"url\":\"" + url + "\"}");
return pluginData;
}

private LettuceConnectionFactory lettuceFactory(final ReactiveRedisTemplate<String, String> template) {
return (LettuceConnectionFactory) template.getConnectionFactory();
}

private RuleData ruleData(final String handle) {
RuleData ruleData = new RuleData();
ruleData.setId("rule-1");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,18 @@ public void handlerPlugin(final PluginData pluginData) {
if (Objects.isNull(REDIS_CACHED_HANDLE.get().obtainHandle(PluginEnum.AI_TOKEN_LIMITER.getName()))
|| Objects.isNull(REDIS_PROPERTIES_CACHED_HANDLE.get().obtainHandle(PluginEnum.AI_TOKEN_LIMITER.getName()))
|| !redisConfigProperties.equals(REDIS_PROPERTIES_CACHED_HANDLE.get().obtainHandle(PluginEnum.AI_TOKEN_LIMITER.getName()))) {
final ReactiveRedisTemplate previousRedisTemplate = REDIS_CACHED_HANDLE.get()
.obtainHandle(PluginEnum.AI_TOKEN_LIMITER.getName());
final RedisConnectionFactory redisConnectionFactory = new RedisConnectionFactory(redisConfigProperties);
ReactiveRedisTemplate<String, String> reactiveRedisTemplate = new ShenyuReactiveRedisTemplate<>(
redisConnectionFactory.getLettuceConnectionFactory(),
ShenyuRedisSerializationContext.stringSerializationContext());
REDIS_CACHED_HANDLE.get().cachedHandle(PluginEnum.AI_TOKEN_LIMITER.getName(), reactiveRedisTemplate);
REDIS_PROPERTIES_CACHED_HANDLE.get().cachedHandle(PluginEnum.AI_TOKEN_LIMITER.getName(), redisConfigProperties);
// The client that is replaced must not keep its connection pool and its threads alive.
if (Objects.nonNull(previousRedisTemplate)) {
RedisConnectionFactory.destroyQuietly(previousRedisTemplate.getConnectionFactory());
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.shenyu.plugin.ai.token.limiter.handler;

import org.apache.shenyu.common.dto.PluginData;
import org.apache.shenyu.common.enums.PluginEnum;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory;
import org.springframework.data.redis.core.ReactiveRedisTemplate;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;

/**
* Test cases for {@link AiTokenLimiterPluginHandler}.
*/
public final class AiTokenLimiterPluginHandlerTest {

@AfterEach
public void tearDown() {
AiTokenLimiterPluginHandler.REDIS_CACHED_HANDLE.get().removeHandle(PluginEnum.AI_TOKEN_LIMITER.getName());
AiTokenLimiterPluginHandler.REDIS_PROPERTIES_CACHED_HANDLE.get()
.removeHandle(PluginEnum.AI_TOKEN_LIMITER.getName());
}

@Test
public void testHandlerPluginCachesTheRedisTemplate() {
new AiTokenLimiterPluginHandler().handlerPlugin(pluginData("127.0.0.1:6379"));
ReactiveRedisTemplate<?, ?> template = redisTemplate();
assertNotNull(template);
assertTrue(lettuceFactory(template).isRunning());
}

@Test
public void testHandlerPluginDestroysTheClientItReplaces() {
AiTokenLimiterPluginHandler handler = new AiTokenLimiterPluginHandler();
handler.handlerPlugin(pluginData("127.0.0.1:6379"));
ReactiveRedisTemplate<?, ?> first = redisTemplate();
assertNotNull(first);

handler.handlerPlugin(pluginData("127.0.0.1:6380"));
ReactiveRedisTemplate<?, ?> second = redisTemplate();
assertNotSame(first, second);
// the client that was replaced must not keep its connection pool and its threads alive
assertFalse(lettuceFactory(first).isRunning());
assertTrue(lettuceFactory(second).isRunning());
}

@Test
public void testHandlerPluginKeepsTheClientWhenTheConfigurationIsUnchanged() {
AiTokenLimiterPluginHandler handler = new AiTokenLimiterPluginHandler();
handler.handlerPlugin(pluginData("127.0.0.1:6379"));
ReactiveRedisTemplate<?, ?> first = redisTemplate();
handler.handlerPlugin(pluginData("127.0.0.1:6379"));
assertSame(first, redisTemplate());
assertTrue(lettuceFactory(first).isRunning());
}

@Test
public void testHandlerPluginDisabledDoesNothing() {
PluginData pluginData = pluginData("127.0.0.1:6379");
pluginData.setEnabled(false);
new AiTokenLimiterPluginHandler().handlerPlugin(pluginData);
assertNull(redisTemplate());
}

private ReactiveRedisTemplate<?, ?> redisTemplate() {
return AiTokenLimiterPluginHandler.REDIS_CACHED_HANDLE.get()
.obtainHandle(PluginEnum.AI_TOKEN_LIMITER.getName());
}

private LettuceConnectionFactory lettuceFactory(final ReactiveRedisTemplate<?, ?> template) {
return (LettuceConnectionFactory) template.getConnectionFactory();
}

private PluginData pluginData(final String url) {
PluginData pluginData = new PluginData();
pluginData.setEnabled(true);
pluginData.setConfig("{\"url\":\"" + url + "\"}");
return pluginData;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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:

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().

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment thread
HY-love-sleep marked this conversation as resolved.
final RedisConnectionFactory redisConnectionFactory = new RedisConnectionFactory(redisConfigProperties);
ReactiveRedisTemplate<String, String> reactiveRedisTemplate = new ShenyuReactiveRedisTemplate<>(
redisConnectionFactory.getLettuceConnectionFactory(),
ShenyuRedisSerializationContext.stringSerializationContext());
Singleton.INST.single(ReactiveRedisTemplate.class, reactiveRedisTemplate);
Singleton.INST.single(RedisConfigProperties.class, redisConfigProperties);
// The client that is replaced must not keep its connection pool and its threads alive.
if (Objects.nonNull(previousRedisTemplate)) {
RedisConnectionFactory.destroyQuietly(previousRedisTemplate.getConnectionFactory());
}
}
}
}
Expand Down
Loading
Loading