-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[type:fix] release the redis client that is replaced #7157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Installing the new template with Two things you may want to fold in later, neither blocking:
|
||
| } | ||
| 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"); | ||
|
|
||
| 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 |
|---|---|---|
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
this.closeCacheIfNeed(); // destroys the old cache here
final ICacheBuilder cacheBuilder = ExtensionLoader...getJoin(...);
Singleton.INST.single(ICache.class, cacheBuilder.builderCache(config)); // new one installed laterThe old hand - ICache lastCache = CacheUtils.getCache(); // Singleton.INST.get(ICache.class)
...
lastCache.close();It never removes Before this change that window was survivable:
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, Note on verification: I could not execute this one. |
||
| } | ||
| } | ||
There was a problem hiding this comment.
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
LettuceConnectionFactorycreated but nobody released - the connection pool and the per-client Netty resources - sodestroy()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:getLettuceClientConfiguration(...)buildsLettucePoolingClientConfiguration.builder().poolConfig(...).build()and never calls.clientResources(...), so the configuration carries no resources.AbstractRedisClientconstructor (decompiled fromlettuce-core-6.3.2.RELEASE.jar):if (clientResources == null) { sharedResources = false; clientResources = DefaultClientResources.create(); }- each client creates and therefore owns its own resources.AbstractRedisClient.closeClientResources(...)only does the fullclientResources.shutdown(...)whensharedResources == 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 understate.compareAndSet(STARTED, STOPPING), so a seconddestroy()is a no-op, andisRunning()is a clean observable to assert on.One note rather than a request: the class is not a Spring bean anywhere, and
RedisConnectionFactoryis not aReactiveRedisConnectionFactoryeither, sodestroyQuietlycan never actually be handed one - every call site passes the innerLettuceConnectionFactory, which has always implementedDisposableBeanon its own. Right now the instancedestroy()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.