Re: [PR] fix(config_center/zookeeper): fix RemoveListener key mismatch + guard listener set [dubbo-go]
Aias00 closed pull request #3540: fix(config_center/zookeeper): fix RemoveListener key mismatch + guard listener set URL: https://github.com/apache/dubbo-go/pull/3540 -- 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]
Re: [PR] fix(config_center/zookeeper): fix RemoveListener key mismatch + guard listener set [dubbo-go]
sonarqubecloud[bot] commented on PR #3540: URL: https://github.com/apache/dubbo-go/pull/3540#issuecomment-5084192105 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3540) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3540&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3540&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3540&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3540&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3540&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3540) -- 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]
Re: [PR] fix(config_center/zookeeper): fix RemoveListener key mismatch + guard listener set [dubbo-go]
zouyx commented on code in PR #3540:
URL: https://github.com/apache/dubbo-go/pull/3540#discussion_r3652821054
##
config_center/zookeeper/listener.go:
##
@@ -44,27 +44,58 @@ func NewCacheListener(rootPath string, listener
*zookeeper.ZkEventListener) *Cac
return &CacheListener{zkEventListener: listener, rootPath: rootPath}
}
+// listenerSet is a mutex-guarded set of ConfigurationListeners. AddListener
and
+// RemoveListener run on router/config goroutines while DataChange runs on the
+// zk event goroutine; guarding the inner map avoids a fatal concurrent map
+// read+write. See #3536.
+type listenerSet struct {
+ musync.Mutex
+ listeners map[config_center.ConfigurationListener]struct{}
+}
+
+func newListenerSet() *listenerSet {
+ return &listenerSet{listeners:
make(map[config_center.ConfigurationListener]struct{})}
+}
+
+func (s *listenerSet) add(l config_center.ConfigurationListener) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ s.listeners[l] = struct{}{}
+}
+
+func (s *listenerSet) remove(l config_center.ConfigurationListener) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ delete(s.listeners, l)
+}
+
+// snapshot returns a slice copy of the listeners under the lock, safe to
+// iterate outside the lock so listener.Process is not called while holding it.
+func (s *listenerSet) snapshot() []config_center.ConfigurationListener {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ out := make([]config_center.ConfigurationListener, 0, len(s.listeners))
+ for l := range s.listeners {
+ out = append(out, l)
+ }
+ return out
+}
+
// AddListener will add a listener if loaded
func (l *CacheListener) AddListener(key string, listener
config_center.ConfigurationListener) {
// FIXME do not use Client.ExistW, cause it has a bug(can not watch zk
node that do not exist)
_, _, _, err := l.zkEventListener.Client.Conn.ExistsW(key)
- // reference from
https://stackoverflow.com/questions/34018908/golang-why-dont-we-have-a-set-datastructure
- // make a map[your type]struct{} like set in java
if err != nil {
return
}
- listeners, loaded := l.keyListeners.LoadOrStore(key,
map[config_center.ConfigurationListener]struct{}{listener: {}})
- if loaded {
-
listeners.(map[config_center.ConfigurationListener]struct{})[listener] =
struct{}{}
- l.keyListeners.Store(key, listeners)
- }
+ actual, _ := l.keyListeners.LoadOrStore(key, newListenerSet())
+ actual.(*listenerSet).add(listener)
}
// RemoveListener will delete a listener if loaded
func (l *CacheListener) RemoveListener(key string, listener
config_center.ConfigurationListener) {
- listeners, loaded := l.keyListeners.Load(key)
- if loaded {
-
delete(listeners.(map[config_center.ConfigurationListener]struct{}), listener)
+ if listeners, ok := l.keyListeners.Load(key); ok {
Review Comment:
是不是 Load 的动作和 Remove 动作在同一个锁内会更合适？
--
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]
Re: [PR] fix(config_center/zookeeper): fix RemoveListener key mismatch + guard listener set [dubbo-go]
codecov-commenter commented on PR #3540: URL: https://github.com/apache/dubbo-go/pull/3540#issuecomment-5084058328 ## [Codecov](https://app.codecov.io/gh/apache/dubbo-go/pull/3540?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :x: Patch coverage is `85.18519%` with `4 lines` in your changes missing coverage. Please review. :white_check_mark: Project coverage is 55.28%. Comparing base ([`60d1c2a`](https://app.codecov.io/gh/apache/dubbo-go/commit/60d1c2a949f0ee0da4be3fe09fb79d295491e040?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`716f8ab`](https://app.codecov.io/gh/apache/dubbo-go/commit/716f8ab192c1b0f1f2cb5672775bb9b1920d3c5e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 875 commits behind head on develop. | [Files with missing lines](https://app.codecov.io/gh/apache/dubbo-go/pull/3540?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [config\_center/zookeeper/impl.go](https://app.codecov.io/gh/apache/dubbo-go/pull/3540?src=pr&el=tree&filepath=config_center%2Fzookeeper%2Fimpl.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29uZmlnX2NlbnRlci96b29rZWVwZXIvaW1wbC5nbw==) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go/pull/3540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [config\_center/zookeeper/listener.go](https://app.codecov.io/gh/apache/dubbo-go/pull/3540?src=pr&el=tree&filepath=config_center%2Fzookeeper%2Flistener.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29uZmlnX2NlbnRlci96b29rZWVwZXIvbGlzdGVuZXIuZ28=) | 90.90% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go/pull/3540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop#3540 +/- ## === + Coverage46.76% 55.28% +8.51% === Files 295 462 +167 Lines1717236012 +18840 === + Hits 803119910 +11879 - Misses828714550+6263 - Partials 854 1552 +698 ``` [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/dubbo-go/pull/3540?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. -- 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]
