Re: [PR] fix(registry/servicediscovery): lock + snapshot listener map in DataChange/callbacks [dubbo-go]
Alanxtl commented on PR #3517: URL: https://github.com/apache/dubbo-go/pull/3517#issuecomment-5086268515 not planned -- 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(registry/servicediscovery): lock + snapshot listener map in DataChange/callbacks [dubbo-go]
Alanxtl closed pull request #3517: fix(registry/servicediscovery): lock + snapshot listener map in DataChange/callbacks URL: https://github.com/apache/dubbo-go/pull/3517 -- 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(registry/servicediscovery): lock + snapshot listener map in DataChange/callbacks [dubbo-go]
codecov-commenter commented on PR #3517: URL: https://github.com/apache/dubbo-go/pull/3517#issuecomment-5082450095 ## [Codecov](https://app.codecov.io/gh/apache/dubbo-go/pull/3517?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 `23.80952%` with `32 lines` in your changes missing coverage. Please review. :white_check_mark: Project coverage is 55.26%. 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 ([`ee8d883`](https://app.codecov.io/gh/apache/dubbo-go/commit/ee8d883910a422fd09e451e7d1169d1e95fb4b61?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/3517?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [registry/etcdv3/service\_discovery.go](https://app.codecov.io/gh/apache/dubbo-go/pull/3517?src=pr&el=tree&filepath=registry%2Fetcdv3%2Fservice_discovery.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVnaXN0cnkvZXRjZHYzL3NlcnZpY2VfZGlzY292ZXJ5Lmdv) | 0.00% | [19 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go/pull/3517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [registry/nacos/service\_discovery.go](https://app.codecov.io/gh/apache/dubbo-go/pull/3517?src=pr&el=tree&filepath=registry%2Fnacos%2Fservice_discovery.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVnaXN0cnkvbmFjb3Mvc2VydmljZV9kaXNjb3ZlcnkuZ28=) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go/pull/3517?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [registry/zookeeper/service\_discovery.go](https://app.codecov.io/gh/apache/dubbo-go/pull/3517?src=pr&el=tree&filepath=registry%2Fzookeeper%2Fservice_discovery.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVnaXN0cnkvem9va2VlcGVyL3NlcnZpY2VfZGlzY292ZXJ5Lmdv) | 71.42% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go/pull/3517?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#3517 +/- ## === + Coverage46.76% 55.26% +8.49% === Files 295 462 +167 Lines1717236019 +18847 === + Hits 803119905 +11874 - Misses828714558+6271 - Partials 854 1556 +702 ``` [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/dubbo-go/pull/3517?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]
Re: [PR] fix(registry/servicediscovery): lock + snapshot listener map in DataChange/callbacks [dubbo-go]
sonarqubecloud[bot] commented on PR #3517: URL: https://github.com/apache/dubbo-go/pull/3517#issuecomment-5082437796 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3517) **Quality Gate passed** Issues  [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3517&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3517&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3517&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3517&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3517&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3517) -- 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(registry/servicediscovery): lock + snapshot listener map in DataChange/callbacks [dubbo-go]
sonarqubecloud[bot] commented on PR #3517: URL: https://github.com/apache/dubbo-go/pull/3517#issuecomment-5082398596 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3517) **Quality Gate passed** Issues  [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3517&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3517&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3517&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3517&metric=new_coverage&view=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3517&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3517) -- 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]
[PR] fix(registry/servicediscovery): lock + snapshot listener map in DataChange/callbacks [dubbo-go]
Aias00 opened a new pull request, #3517: URL: https://github.com/apache/dubbo-go/pull/3517 ## What The Zookeeper / etcdv3 / Nacos service-discovery backends read `instanceListenerMap[serviceName]` inside their watcher/subscription callbacks **without holding the lock** that `AddListener` / `registerInstanceListener` / `Destroy` mutate it under. Concurrent subscribe/unsubscribe therefore triggers a fatal `concurrent map read and map write`. ## Why - **zookeeper** `registry/zookeeper/service_discovery.go:279` — `DataChange` iterates `zksd.instanceListenerMap[serviceName].Values()` with no `listenLock`, while `AddListener` (`:238`) and (via `Destroy`) mutate the map under `listenLock`. - **etcdv3** `registry/etcdv3/service_discovery.go:290` — `DataChange` reads `e.instanceListenerMap[...]` with no `initLock`, while `registerServiceInstanceListener`/`registerServiceWatcher` write under `initLock`. Additionally, the decode-error path blanked `ServiceName` to `""` and then looked up `instanceListenerMap[""]` — a silent no-op rather than a real failure. - **nacos** `registry/nacos/service_discovery.go:320` — the `SubscribeCallback` reads `n.instanceListenerMap[serviceName]` with no `listenerLock`, while `registerInstanceListener` and `Destroy` mutate it under `listenerLock`; an in-flight callback delivered after `Destroy` clears the map is a live race. (`gxset.HashSet.Values()` does not panic on a nil receiver, so the issue is the concurrent map access, not a nil deref.) ## Fix Snapshot the listener set under the writer lock, then iterate the snapshot **outside** the lock to dispatch — this both eliminates the race and stops a slow `OnEvent` from blocking every other listener: - **zookeeper** `DataChange`: snapshot under `listenLock` via a new `snapshotListeners` helper; dispatch outside the lock. - **etcdv3** `DataChange`: snapshot under `initLock`; on decode error, log and `return false` instead of blanking `ServiceName`. - **nacos** `SubscribeCallback`: snapshot under `listenerLock` before `OnEvent`. ## Tests Added `TestZookeeperServiceDiscovery_SnapshotListeners` covering: missing key (safe no-op), present key (returns registered listeners), and concurrent snapshot-vs-mutation race-freedom under `-race`. `registry/zookeeper`, `registry/etcdv3`, `registry/nacos` all pass under `-race`. Fixes #3512 -- 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]
