AlexStocks commented on code in PR #982:
URL: https://github.com/apache/dubbo-go-pixiu/pull/982#discussion_r4180468076
##########
pkg/adapter/dubboregistry/registry/nacos/registry.go:
##########
@@ -55,7 +55,22 @@ func (n *NacosRegistry) DoSubscribe() error {
}
func (n *NacosRegistry) DoUnsubscribe() error {
- panic("implement me")
+ // Stop the background listener first (it drains its watch goroutine),
then
Review Comment:
[P1] 这个 PR 与 #980 在同一个函数上有必然的文本冲突，而且两版改法不兼容——合并顺序会让其中一半逻辑静默丢失。
`DoUnsubscribe` 在 base 里是 `panic("implement me")`，本 PR 和 #980 都从这一行出发改写它，所以
git 一定报冲突。真正的风险在于**解决冲突时只能保留一版**，而两版各自都有对方没有的内容：
| | 本 PR (#982) | #980 |
|---|---|---|
| `BaseRegistry == nil` 守卫 | 无 | 有 |
| interface listener `Close()` | 有 | 有 |
| svc listener 清理 + `RemoveSvcListener` | 有 | **无** |
| `n.client.CloseClient()` | 有 | **无** |
实测两边的 hunk（`git diff ba018888 ba018888...HEAD -- .../registry.go`）：
- #980：`if n.BaseRegistry == nil { return errors.New(...) }` +
`listener.Close(); return nil`
- #982（本 PR）：`Listener.Close()` + `for k, l := range n.GetAllSvcListener() {
l.Close(); n.RemoveSvcListener(k) }` + `n.client.CloseClient()`
两个 PR 的完整文件重叠是 **10 个**，包括
`registry.go`、`application_listener.go`、`interface_listener.go`、`service_listener.go`
以及 4 个测试文件。
后果按合并顺序分两种：
- **先 #980 后 #982**（或反之），解冲突时若取了另一侧的 `DoUnsubscribe`，则 `CloseClient()` 或 svc
listener 清理会整块消失——而 `CloseClient` 恰好是本 PR 修复泄漏的关键手段（你在 commit `0e94c1ee`
里专门补的），退回 v1 的 `panic` 等于这个修复整体失效；
- `#982 ∩ #994` 还重叠了 `pkg/cmd/gateway.go`（本 PR 改了 `server.Start` 的错误处理，#994
改了 `start()` 的返回）。这个重叠较小，但同样需要人工确认。
建议：由 #982 作为 Nacos 迁移的主 PR 先合（它把 import 从 v1 切到 v2，#980 的改动建立在 v1 API
之上，顺序上后者自然跟随），然后 #980 rebase 到本 PR 之上，再手工把 `BaseRegistry == nil` 守卫和 svc
listener 清理合并进同一份 `DoUnsubscribe`。合完后 `CloseClient()`、`GetAllSvcListener()`
清理、`BaseRegistry` 守卫三者应同时存在。
顺带一提，本 PR 的 `DoUnsubscribe` 早退分支（`!ok` 时直接 return）会跳过后面所有的 `Close` 和
`CloseClient`。我核实过它在当前代码里不可达——`RegisterTypeFromName` 的 default 返回
`RegisteredTypeInterface`（非零值），且 `newNacosRegistry` 在 `default` 分支就会先返回错误，所以
`nacosListeners` 构造成功时一定有对应项。但如果以后有别的注册路径绕过这个 switch，这个早退就会变成"listener 缺失 ⇒ 顺带把
client 也泄漏了"，建议把 `CloseClient()` 放到 `defer` 或早退之前。
你这次对 @AlexStocks 那三条 P1/P2 的处理我都核实过，是真修了：
- `CloseClient` 已补齐 5
个创建点（`configcenter/nacos_load.go:172`、`dubboregistry/registry/nacos/registry.go:72`、`llmregistry/registry/nacos/registry.go:59`、`remote/nacos/client.go:66`，加上
MCP 适配器原有的），`DoUnsubscribe` 里确实调到了；
- `cutServiceName` 已从测试删除（`grep -rn cutServiceName`
无命中），`TestCallback_ServiceNameWithGroupPrefix` 直接调生产 `Callback`，符合我提的"别复制实现"；
- `strings.Cut` 和 `GetUniqKey()` 复用都已落地。
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
