This is an automated email from the ASF dual-hosted git repository.
Aias00 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/shenyu.git
The following commit(s) were added to refs/heads/master by this push:
new ea28bde23e fix(admin): guard removeProxySelector when the discovery
was removed first (#7365)
ea28bde23e is described below
commit ea28bde23ed7171b01d82b0fa9c7dcc9d6e58d1f
Author: Sean-Walker0 <[email protected]>
AuthorDate: Wed Sep 30 11:15:41 2026 +0800
fix(admin): guard removeProxySelector when the discovery was removed first
(#7365)
* fix(admin): guard removeProxySelector when the discovery was removed first
removeDiscovery drops only the discoveryServiceCache entry and leaves
the dataChangedEventListenerCache entry (registered by
createProxySelector) behind. removeProxySelector then finds the stale
listener-cache key, runs its lambda, and calls
unWatchInstances on a service that discoveryServiceCache.get just
returned null for - so removing a proxy selector after its discovery
was deleted throws NullPointerException and the PROXY_SELECTOR delete
event is never published. Skip only the moot unwatch when the service
is already gone: the listener key is still removed and the delete event
still fires.
The new test fails on current master with the exact NPE and passes
with this change.
* fix(admin): fold review suggestions into the removeProxySelector guard
Per review: use Optional.ifPresent for the unwatch call to match the
idiom of the surrounding code; drop the emptied listener-cache set so
the map converges instead of keeping a tombstone entry; parameterize
the test's reflective casts. The empty-set cleanup alone would break
re-registration (getCacheKey returned the raw map entry and both
createProxySelector variants call cacheKey.add on it), so getCacheKey
now recreates the set via computeIfAbsent - unregistered discoveries
are still rejected by the service null check above. The test now also
asserts the tombstone is gone and re-registration gets a usable set.
---------
Co-authored-by: Sean-Walker0
<[email protected]>
Co-authored-by: xiaoyu <[email protected]>
---
.../discovery/AbstractDiscoveryProcessor.java | 11 ++++++--
.../discovery/DefaultDiscoveryProcessorTest.java | 30 ++++++++++++++++++++++
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git
a/shenyu-admin/src/main/java/org/apache/shenyu/admin/discovery/AbstractDiscoveryProcessor.java
b/shenyu-admin/src/main/java/org/apache/shenyu/admin/discovery/AbstractDiscoveryProcessor.java
index 807da75826..9606b3518c 100644
---
a/shenyu-admin/src/main/java/org/apache/shenyu/admin/discovery/AbstractDiscoveryProcessor.java
+++
b/shenyu-admin/src/main/java/org/apache/shenyu/admin/discovery/AbstractDiscoveryProcessor.java
@@ -132,7 +132,13 @@ public abstract class AbstractDiscoveryProcessor
implements DiscoveryProcessor,
String key =
buildProxySelectorKey(discoveryHandlerDTO.getListenerNode());
Optional.ofNullable(dataChangedEventListenerCache.get(discoveryHandlerDTO.getDiscoveryId())).ifPresent(cacheKey
-> {
cacheKey.remove(key);
- shenyuDiscoveryService.unWatchInstances(key);
+ if (cacheKey.isEmpty()) {
+ // converge the map instead of leaving an empty tombstone set
behind forever
+
dataChangedEventListenerCache.remove(discoveryHandlerDTO.getDiscoveryId());
+ }
+ // removeDiscovery drops the service entry but leaves the
listener-cache key behind;
+ // with the service already closed there is nothing to unwatch,
but the delete event must still fire
+ Optional.ofNullable(shenyuDiscoveryService).ifPresent(service ->
service.unWatchInstances(key));
DataChangedEvent dataChangedEvent = new
DataChangedEvent(ConfigGroupEnum.PROXY_SELECTOR, DataEventTypeEnum.DELETE,
Collections.singletonList(DiscoveryTransfer.INSTANCE.mapToData(proxySelectorDTO)));
eventPublisher.publishEvent(dataChangedEvent);
@@ -286,7 +292,8 @@ public abstract class AbstractDiscoveryProcessor implements
DiscoveryProcessor,
* @return set
*/
public Set<String> getCacheKey(final String discoveryId) {
- return dataChangedEventListenerCache.get(discoveryId);
+ // computeIfAbsent keeps re-registration safe after
removeProxySelector dropped an emptied set
+ return dataChangedEventListenerCache.computeIfAbsent(discoveryId, k ->
new HashSet<>());
}
/**
diff --git
a/shenyu-admin/src/test/java/org/apache/shenyu/admin/discovery/DefaultDiscoveryProcessorTest.java
b/shenyu-admin/src/test/java/org/apache/shenyu/admin/discovery/DefaultDiscoveryProcessorTest.java
index 819cce7028..778dff6b74 100644
---
a/shenyu-admin/src/test/java/org/apache/shenyu/admin/discovery/DefaultDiscoveryProcessorTest.java
+++
b/shenyu-admin/src/test/java/org/apache/shenyu/admin/discovery/DefaultDiscoveryProcessorTest.java
@@ -45,6 +45,7 @@ import org.springframework.context.ApplicationEventPublisher;
import java.lang.reflect.Field;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
@@ -53,6 +54,8 @@ import java.util.Properties;
import java.util.Set;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doNothing;
@@ -199,4 +202,31 @@ public class DefaultDiscoveryProcessorTest {
}
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testRemoveProxySelectorAfterDiscoveryRemoved() throws
NoSuchFieldException, IllegalAccessException {
+ defaultDiscoveryProcessor.setApplicationEventPublisher(eventPublisher);
+ // simulate removeDiscovery: it drops only the discoveryServiceCache
entry and leaves the
+ // listener-cache key registered by createProxySelector behind
+ final Field serviceCacheField =
defaultDiscoveryProcessor.getClass().getSuperclass().getDeclaredField("discoveryServiceCache");
+ serviceCacheField.setAccessible(true);
+ Map<String, ShenyuInstanceRegisterRepository> serviceCache =
(Map<String, ShenyuInstanceRegisterRepository>)
serviceCacheField.get(defaultDiscoveryProcessor);
+ serviceCache.remove("id");
+ final Field listenerCacheField =
defaultDiscoveryProcessor.getClass().getSuperclass().getDeclaredField("dataChangedEventListenerCache");
+ listenerCacheField.setAccessible(true);
+ Map<String, Set<String>> listenerCache = (Map<String, Set<String>>)
listenerCacheField.get(defaultDiscoveryProcessor);
+ listenerCache.put("id", new
HashSet<>(Collections.singleton("/shenyu/discovery")));
+
+
doNothing().when(eventPublisher).publishEvent(any(DataChangedEvent.class));
+ DiscoveryHandlerDTO discoveryHandlerDTO = new DiscoveryHandlerDTO();
+ discoveryHandlerDTO.setDiscoveryId("id");
+ discoveryHandlerDTO.setListenerNode("/shenyu/discovery");
+ defaultDiscoveryProcessor.removeProxySelector(discoveryHandlerDTO, new
ProxySelectorDTO());
+ verify(eventPublisher).publishEvent(any(DataChangedEvent.class));
+ // the emptied set must not linger as a tombstone entry
+ assertTrue(listenerCache.isEmpty());
+ // and re-registering the selector must get a usable set again instead
of an NPE
+
assertFalse(defaultDiscoveryProcessor.getCacheKey("id").contains("/shenyu/discovery"));
+ }
+
}
