bugfix：修复 nacos client 不会回调 listener 方法的问题 #13341
Conversation
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
添加强制通知参数修复Nacos客户端监听器不回调问题变更文件
时序图sequenceDiagram
participant SC as ServiceInfoHolder
participant NCPD as NamingClientProxyDelegate
participant SIPU as ServiceInfoUpdateService
participant NPH as NamingPushRequestHandler
NCPD->>SC: processServiceInfo(serviceInfo, true)
SIPU->>SC: processServiceInfo(serviceInfo, false)
NPH->>SC: processServiceInfo(serviceInfo, false)
SC->>NotifyCenter: publishEvent(InstancesChangeEvent) when forceNotify || diff.hasDifferent()
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 1 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 2 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 3 个问题
⚠️ **需要立即关注的阻断性问题**
client/src/main/java/com/alibaba/nacos/client/naming/remote/NamingClientProxyDelegate.java
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ client/src/main/java/com/alibaba/nacos/client/naming/cache/ServiceInfoHolder.java (1 💬)
☕ client/src/main/java/com/alibaba/nacos/client/naming/remote/NamingClientProxyDelegate.java (1 💬)
☕ client/src/test/java/com/alibaba/nacos/client/naming/cache/ServiceInfoHolderTest.java (1 💬)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. processServiceInfo方法参数变更导致的跨文件调用不一致风险
在ServiceInfoHolder类中新增的forceNotify参数改变了原有方法签名，但部分调用点未同步更新参数传递逻辑。例如NamingClientProxyDelegate.subscribe方法传入true，而ServiceInfoUpdateService和NamingPushRequestHandler传入false。需要统一forceNotify参数的使用策略，确保所有调用场景的参数传递符合设计预期。
📌 关键代码：
serviceInfoHolder.processServiceInfo(result, true);serviceInfoHolder.processServiceInfo(serviceObj, false);🔍 2. serviceKey空值处理的系统性逻辑漏洞
新增的serviceKey空值处理逻辑在forceNotify为true时强制发布事件，但未检查serviceInfo的有效性。若传入空ServiceInfo对象且forceNotify为true，可能触发空指针异常或无效事件。需在ServiceInfoHolder.processServiceInfo方法入口增加参数有效性校验。
📌 关键代码：
if (serviceKey == null) { ... }🔍 3. 测试覆盖范围不完整
虽然新增了forceNotify=true且无差异的测试用例，但缺乏对forceNotify=true但存在数据差异的测试场景。需补充同时满足forceNotify=true和diff.hasDifferent()=true的测试案例，确保双重条件判断的正确性。
📌 关键代码：
testProcessNullServiceInfo()等测试用例🔍 4. 方法重载导致的潜在代码冗余
新增的processServiceInfo(String json, boolean forceNotify)和processServiceInfo(ServiceInfo, boolean forceNotify)存在重复逻辑，建议将JSON解析后的公共逻辑提取到私有方法中，避免代码重复和维护成本增加。
📌 关键代码：
public ServiceInfo processServiceInfo(String json, boolean forceNotify) { ... }public ServiceInfo processServiceInfo(ServiceInfo serviceInfo, boolean forceNotify) { ... }🔍 5. Metrics监控的异常处理风险
在enableClientMetrics分支中，MetricsMonitor的异常捕获未明确限制异常类型。建议将catch(Throwable)改为catch(Exception)，避免吞噬Error类型异常（如OutOfMemoryError），防止系统级异常被错误处理。
📌 关键代码：
catch (Throwable t) { ... }💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| if (forceNotify) { | ||
| InstancesDiff diff = getServiceInfoDiff(null, oldService); | ||
| NotifyCenter.publishEvent( | ||
| new InstancesChangeEvent(notifierEventScope, oldService.getName(), oldService.getGroupName(), | ||
| oldService.getClusters(), oldService.getHosts(), diff)); | ||
| } |
There was a problem hiding this comment.
修复ServiceInfoHolder在serviceKey为null时可能触发无效事件通知的问题
🟡 Major | 🐞 Bugs
📋 问题详情
在processServiceInfo方法中，当serviceKey为null时，新增的forceNotify分支直接使用oldService参数调用NotifyCenter.publishEvent。但此时oldService可能未初始化（如首次调用），导致传入无效参数。这可能导致事件监听器收到不完整的数据，影响下游服务发现逻辑。
💡 解决方案
需要确保oldService非空后再触发事件：
+ if (oldService != null && forceNotify) {
- if (forceNotify) {
InstancesDiff diff = getServiceInfoDiff(null, oldService);
NotifyCenter.publishEvent(
new InstancesChangeEvent(notifierEventScope, oldService.getName(), oldService.getGroupName(),
oldService.getClusters(), oldService.getHosts(), diff));
+ }确保在首次调用时oldService未初始化时不会触发无效事件。
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| @@ -172,7 +172,7 @@ public ServiceInfo subscribe(String serviceName, String groupName, String cluste | |||
| if (null == result || !isSubscribed(serviceName, groupName, clusters)) { | |||
| result = grpcClientProxy.subscribe(serviceName, groupName, clusters); | |||
| } | |||
| serviceInfoHolder.processServiceInfo(result); | |||
| serviceInfoHolder.processServiceInfo(result, true); | |||
There was a problem hiding this comment.
修正processServiceInfo方法调用参数传递问题
🔴 Blocker | 🐞 Bugs
📋 问题详情
在subscribe方法中调用processServiceInfo时传递了false作为forceNotify参数。根据设计意图，订阅操作应强制触发监听器通知，此处应传递true以确保事件触发。
💡 解决方案
应使用true确保订阅时触发事件：
- serviceInfoHolder.processServiceInfo(result, false);
+ serviceInfoHolder.processServiceInfo(result, true);确保客户端订阅时强制通知监听器更新服务信息。
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
|
|
||
| assertEquals(oldInfo.getKey(), actual.getKey()); | ||
| assertEquals(2, actual.getHosts().size()); | ||
| } | ||
|
|
||
| @Test | ||
| void testProcessNullServiceInfo() { | ||
| assertNull(holder.processServiceInfo(new ServiceInfo())); | ||
| assertNull(holder.processServiceInfo(new ServiceInfo(), false)); |
There was a problem hiding this comment.
补充测试用例覆盖forceNotify为true且无差异的场景
🟡 Major | 📝 Unit test coverage
📋 问题详情
现有测试用例未覆盖当forceNotify为true但实例差异为false时触发事件的场景，可能导致该逻辑未经过充分测试。
💡 解决方案
新增测试用例验证forceNotify强制触发：
+ @Test
+ void testForceNotifyWithoutDiff() {
+ ServiceInfo oldInfo = new ServiceInfo("test");
+ serviceInfoHolder.processServiceInfo(oldInfo, false);
+ ServiceInfo newInfo = new ServiceInfo("test");
+ serviceInfoHolder.processServiceInfo(newInfo, true);
+ verify(NotifyCenter).publishEvent(any(InstancesChangeEvent.class));
+ }验证即使无差异时forceNotify=true仍会触发事件。
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
|
|
|
麻烦升级一下客户端版本， 这个问题应该已经修复了。 |
嗯嗯，我测试下看看 |
问题描述
当订阅的时候，Nacos client 有可能不会回调 listener 方法
复现步骤：
1、执行 getAllInstance 方法，触发 subscribe 逻辑，更新 ServiceInfoHolder 的数据；
2、执行 subscribe 方法， 手动订阅，再次更新 ServiceInfoHolder 的数据；
3、由于步骤1和步骤2中，ServiceInfoHolder 的数据一致，所以步骤2中不会触发 listener 方法的回调。详细流程如下图：
解决思路
添加一个参数，设置是否强制通知 listener 方法。比如，当用户手动执行 subscribe 方法的时候，强制回到 listener 方法，确保会走用户的回调逻辑