fix(config_center/nacos): remove unused context.WithCancel and call CancelListenConfig on last listener removal#3441
Conversation
…ancelListenConfig on last listener removal
The addListener() function created context.WithCancel but the cancel
function was never called, causing a goroutine and context leak.
Additionally, removeListener() never called CancelListenConfig to
unsubscribe from the nacos server, leaving stale config change
listeners active indefinitely.
Changes:
- Remove unused context.WithCancel calls in addListener()
- Replace context.CancelFunc value type with struct{} in keyListeners
map (the cancel function was never used)
- In removeListener(), call CancelListenConfig when the last listener
for a key is removed (matching Apollo and File implementations)
- Clean up keyListeners entry when no listeners remain
- Add nil client guard for test compatibility
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors Nacos listener bookkeeping by removing per-listener context.CancelFunc storage and adds logic to cancel the underlying Nacos subscription when the last listener for a key is removed.
Changes:
- Stop creating/storing
context.WithCancelcancel funcs for each listener and store astruct{}placeholder instead. - Enhance
removeListenerto delete the key entry and callCancelListenConfigwhen no listeners remain. - Update
keyListenerstype comment to reflect the new stored value type.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| config_center/nacos/listener.go | Removes context usage, stores listeners as set entries, and cancels Nacos subscription when a key has no listeners. |
| config_center/nacos/impl.go | Updates keyListeners comment to reflect struct{} placeholder values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import ( | ||
| "context" | ||
| "sync" | ||
| ) |
There was a problem hiding this comment.
Thanks for the review! These imports are already present in the file — the diff only showed the first import block, but the file has three import blocks (lines 20-22, 24-29, 31-37). The code compiles and all tests pass. No change needed.
| err := n.client.Client().CancelListenConfig(vo.ConfigParam{ | ||
| DataId: key, | ||
| Group: n.resolvedGroup(n.url.GetParam(constant.NacosGroupKey, constant2.DEFAULT_GROUP)), | ||
| }) |
| 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) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Good catch! Fixed in the latest commit. I replaced the bare sync.Map with a keyListenerSet struct that uses sync.Mutex to make add/remove/check-empty atomic. This prevents the race where a concurrent addListener could add a listener between the emptiness check and the CancelListenConfig call. Additionally, I now delete from keyListeners before calling CancelListenConfig so that a new addListener won't find the stale set.
| err := n.client.Client().CancelListenConfig(vo.ConfigParam{ | ||
| DataId: key, | ||
| Group: n.resolvedGroup(n.url.GetParam(constant.NacosGroupKey, constant2.DEFAULT_GROUP)), | ||
| }) |
There was a problem hiding this comment.
Valid concern. Fixed in the latest commit — the resolved group string is now stored in the keyListenerSet struct at registration time, and CancelListenConfig uses set.group directly instead of re-deriving it. This guarantees the same (DataId, Group) pair is used for both ListenConfig and CancelListenConfig.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3441 +/- ##
===========================================
+ Coverage 46.76% 53.58% +6.81%
===========================================
Files 295 493 +198
Lines 17172 38418 +21246
===========================================
+ Hits 8031 20586 +12555
- Misses 8287 16180 +7893
- Partials 854 1652 +798 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…fe listener management Address Copilot review feedback: 1. Race condition: Replace bare sync.Map with keyListenerSet struct that uses sync.Mutex to make add/remove/check-empty atomic, preventing concurrent addListener from racing with removeListener's emptiness check and CancelListenConfig call. 2. DataId/Group consistency: Store the resolved group string in keyListenerSet so that CancelListenConfig uses the exact same group that was passed to ListenConfig, avoiding mismatch from re-derivation. 3. Delete keyListeners entry before CancelListenConfig to prevent new addListener from finding a stale set after cancellation. 4. Use snapshot() for callback iteration to safely iterate while mutations may occur. Also add test for multi-listener removal behavior.
|
Problem
addListener()inconfig_center/nacos/listener.gocreatedcontext.WithCancel(context.Background())but the returnedcancelfunction was never called, causing a goroutine and context leak. Additionally,removeListener()never calledCancelListenConfigto unsubscribe from the nacos server, leaving stale config change listeners active indefinitely even after all listeners were removed.Changes
config_center/nacos/listener.gocontext.WithCancelcalls inaddListener()— the cancel functions were stored but never invoked, leaking contexts and goroutinescontext.CancelFuncvalue type withstruct{}in the innersync.Mapsince the cancel function served no purpose"context"importremoveListener(), callCancelListenConfigwhen the last listener for a key is removed, matching the behavior of Apollo (RemoveChangeListener) and File (watch.Remove) implementationskeyListenersentry when no listeners remain for a keyconfig_center/nacos/impl.gokeyListenerstype comment fromcontext.CancelFunctostruct{}Before vs After
addListenercreates contextcontext.WithCancelcalled but cancel never usedstruct{}{}storedremoveListenerremoves last listenerCancelListenConfigTesting
go test ./config_center/nacos/...)go build ./...)🤖 Generated with Claude Code