Copilot commented on code in PR #3441:
URL: https://github.com/apache/dubbo-go/pull/3441#discussion_r3426355743
##########
config_center/nacos/listener.go:
##########
@@ -70,17 +68,35 @@ func (n *nacosDynamicConfiguration) addListener(key string,
listener config_cent
return
}
}
- _, cancel := context.WithCancel(context.Background())
listenersMap := rawListenersMap.(*sync.Map)
- listenersMap.Store(listener, cancel)
+ listenersMap.Store(listener, struct{}{})
}
func (n *nacosDynamicConfiguration) removeListener(key string, listener
config_center.ConfigurationListener) {
rawListenersMap, loaded := n.keyListeners.Load(key)
if !loaded {
logger.Errorf("[ConfigCenter][Nacos] key is not be listened,
key=%s", key)
- } else {
- listenersMap := rawListenersMap.(*sync.Map)
- listenersMap.Delete(listener)
+ return
+ }
+ listenersMap := rawListenersMap.(*sync.Map)
+ listenersMap.Delete(listener)
+
+ // If no listeners remain for this key, cancel the nacos config
subscription
+ isEmpty := true
+ listenersMap.Range(func(_, _ any) bool {
+ isEmpty = false
+ return false
+ })
+ if isEmpty {
+ n.keyListeners.Delete(key)
+ if n.client != nil {
+ err :=
n.client.Client().CancelListenConfig(vo.ConfigParam{
+ DataId: key,
+ Group:
n.resolvedGroup(n.url.GetParam(constant.NacosGroupKey,
constant2.DEFAULT_GROUP)),
+ })
Review Comment:
`listener.go` now references `vo`, `constant`, and `constant2`, but the
shown import block only imports `sync`. As written, this hunk will not compile
unless those packages are imported elsewhere in this file; add the required
imports in this file’s import block (and run gofmt) so the new
`CancelListenConfig` call builds.
##########
config_center/nacos/listener.go:
##########
@@ -70,17 +68,35 @@ func (n *nacosDynamicConfiguration) addListener(key string,
listener config_cent
return
}
}
- _, cancel := context.WithCancel(context.Background())
listenersMap := rawListenersMap.(*sync.Map)
- listenersMap.Store(listener, cancel)
+ listenersMap.Store(listener, struct{}{})
}
func (n *nacosDynamicConfiguration) removeListener(key string, listener
config_center.ConfigurationListener) {
rawListenersMap, loaded := n.keyListeners.Load(key)
if !loaded {
logger.Errorf("[ConfigCenter][Nacos] key is not be listened,
key=%s", key)
- } else {
- listenersMap := rawListenersMap.(*sync.Map)
- listenersMap.Delete(listener)
+ return
+ }
+ listenersMap := rawListenersMap.(*sync.Map)
+ listenersMap.Delete(listener)
+
+ // If no listeners remain for this key, cancel the nacos config
subscription
+ isEmpty := true
+ listenersMap.Range(func(_, _ any) bool {
+ isEmpty = false
+ return false
+ })
+ if isEmpty {
+ n.keyListeners.Delete(key)
+ if n.client != nil {
+ err :=
n.client.Client().CancelListenConfig(vo.ConfigParam{
+ DataId: key,
+ Group:
n.resolvedGroup(n.url.GetParam(constant.NacosGroupKey,
constant2.DEFAULT_GROUP)),
+ })
Review Comment:
`CancelListenConfig` requires the same `(DataId, Group)` pair that was used
to register the listener. Passing `DataId: key` while deriving `Group` from URL
defaults can be incorrect if `key` is a composite “listenKey” (common in Nacos
integrations) or if the subscription group varies per key. To avoid failing to
cancel (leaking subscriptions) or canceling the wrong subscription, store the
actual `vo.ConfigParam` (or at least `dataId` and `group`) alongside `key` when
registering, and use that exact pair when canceling.
##########
config_center/nacos/listener.go:
##########
@@ -18,7 +18,6 @@
package nacos
import (
- "context"
"sync"
)
Review Comment:
`listener.go` now references `vo`, `constant`, and `constant2`, but the
shown import block only imports `sync`. As written, this hunk will not compile
unless those packages are imported elsewhere in this file; add the required
imports in this file’s import block (and run gofmt) so the new
`CancelListenConfig` call builds.
##########
config_center/nacos/listener.go:
##########
@@ -70,17 +68,35 @@ func (n *nacosDynamicConfiguration) addListener(key string,
listener config_cent
return
}
}
- _, cancel := context.WithCancel(context.Background())
listenersMap := rawListenersMap.(*sync.Map)
- listenersMap.Store(listener, cancel)
+ listenersMap.Store(listener, struct{}{})
}
func (n *nacosDynamicConfiguration) removeListener(key string, listener
config_center.ConfigurationListener) {
rawListenersMap, loaded := n.keyListeners.Load(key)
if !loaded {
logger.Errorf("[ConfigCenter][Nacos] key is not be listened,
key=%s", key)
- } else {
- listenersMap := rawListenersMap.(*sync.Map)
- listenersMap.Delete(listener)
+ return
+ }
+ listenersMap := rawListenersMap.(*sync.Map)
+ listenersMap.Delete(listener)
+
+ // If no listeners remain for this key, cancel the nacos config
subscription
+ isEmpty := true
+ listenersMap.Range(func(_, _ any) bool {
+ isEmpty = false
+ return false
+ })
+ if isEmpty {
+ n.keyListeners.Delete(key)
+ if n.client != nil {
+ err :=
n.client.Client().CancelListenConfig(vo.ConfigParam{
+ DataId: key,
+ Group:
n.resolvedGroup(n.url.GetParam(constant.NacosGroupKey,
constant2.DEFAULT_GROUP)),
+ })
+ if err != nil {
+ logger.Errorf("[ConfigCenter][Nacos] cancel
listen config fail, key=%s, err=%v", key, err)
+ }
+ }
}
Review Comment:
The “delete → range to check empty → delete key → cancel” sequence is not
atomic. A concurrent `addListener` can add a listener after the emptiness check
(or during it), yet this code may still delete `keyListeners[key]` and call
`CancelListenConfig`, potentially dropping active listeners or canceling Nacos
pushes while someone is re-subscribing. Consider introducing per-key
synchronization (e.g., store a per-key struct containing a `sync.Mutex` +
listener set/refcount, or guard add/remove with a shared lock) so that checking
emptiness and canceling/unregistering happens atomically with respect to
adds/removes.
--
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]
