utafrali commented on code in PR #7365:
URL: https://github.com/apache/shenyu/pull/7365#discussion_r4130196391
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/discovery/AbstractDiscoveryProcessor.java:
##########
@@ -132,7 +132,11 @@ public void removeProxySelector(final DiscoveryHandlerDTO
discoveryHandlerDTO, f
String key =
buildProxySelectorKey(discoveryHandlerDTO.getListenerNode());
Optional.ofNullable(dataChangedEventListenerCache.get(discoveryHandlerDTO.getDiscoveryId())).ifPresent(cacheKey
-> {
cacheKey.remove(key);
- shenyuDiscoveryService.unWatchInstances(key);
+ // 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
+ if (Objects.nonNull(shenyuDiscoveryService)) {
+ shenyuDiscoveryService.unWatchInstances(key);
Review Comment:
The null guard works, but consider replacing the `if (Objects.nonNull(...))`
block with `Optional.ofNullable(shenyuDiscoveryService).ifPresent(svc ->
svc.unWatchInstances(key))`. It's more consistent with the outer
`Optional.ofNullable(...).ifPresent(...)` idiom already in use two lines above,
and avoids introducing a new control-flow style inside the lambda.
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/discovery/AbstractDiscoveryProcessor.java:
##########
@@ -132,7 +132,11 @@ public void removeProxySelector(final DiscoveryHandlerDTO
discoveryHandlerDTO, f
String key =
buildProxySelectorKey(discoveryHandlerDTO.getListenerNode());
Optional.ofNullable(dataChangedEventListenerCache.get(discoveryHandlerDTO.getDiscoveryId())).ifPresent(cacheKey
-> {
cacheKey.remove(key);
- shenyuDiscoveryService.unWatchInstances(key);
+ // removeDiscovery drops the service entry but leaves the
listener-cache key behind;
Review Comment:
After `cacheKey.remove(key)`, if this was the last key in the set, an empty
`HashSet` entry for `discoveryId` stays in `dataChangedEventListenerCache`
forever (until `createDiscovery` overwrites it with a fresh set). Neither this
PR nor `removeDiscovery` cleans it up. This is a pre-existing minor leak, but
now that this code path is being touched it would be clean to add `if
(cacheKey.isEmpty()) {
dataChangedEventListenerCache.remove(discoveryHandlerDTO.getDiscoveryId()); }`
after the remove — that way the map converges back to an empty state rather
than accumulating tombstone entries.
##########
shenyu-admin/src/test/java/org/apache/shenyu/admin/discovery/DefaultDiscoveryProcessorTest.java:
##########
@@ -199,4 +200,26 @@ public void testRemoveDiscovery() {
}
+ @Test
+ 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> listenerCache = (Map<String, Set>)
listenerCacheField.get(defaultDiscoveryProcessor);
Review Comment:
The cast target uses the raw type `Map<String, Set>` (and the inner `Set` is
also raw). The actual field is `Map<String, Set<String>>`. Prefer the
parameterized form with `@SuppressWarnings("unchecked")` to keep the compiler
happy without masking the types:
```java
@SuppressWarnings("unchecked")
Map<String, Set<String>> listenerCache =
(Map<String, Set<String>>)
listenerCacheField.get(defaultDiscoveryProcessor);
```
The same applies to the `serviceCache` cast a few lines earlier, though that
one already uses the full type parameter.
--
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]
