Alanxtl commented on code in PR #3367:
URL: https://github.com/apache/dubbo-go/pull/3367#discussion_r3353337550
##########
metadata/metadata.go:
##########
@@ -34,25 +39,33 @@ func GetMetadataService() MetadataService {
}
func GetMetadataInfo(registryId string) *info.MetadataInfo {
+ registryMetadataLock.RLock()
+ defer registryMetadataLock.RUnlock()
return registryMetadataInfo[registryId]
}
func AddService(registryId string, url *common.URL) {
+ registryMetadataLock.Lock()
if _, exist := registryMetadataInfo[registryId]; !exist {
registryMetadataInfo[registryId] = info.NewMetadataInfo(
url.GetParam(constant.ApplicationKey, ""),
url.GetParam(constant.ApplicationTagKey, ""),
)
}
+ registryMetadataLock.Unlock()
+
registryMetadataInfo[registryId].AddService(url)
}
func AddSubscribeURL(registryId string, url *common.URL) {
+ registryMetadataLock.Lock()
if _, exist := registryMetadataInfo[registryId]; !exist {
registryMetadataInfo[registryId] = info.NewMetadataInfo(
url.GetParam(constant.ApplicationKey, ""),
url.GetParam(constant.ApplicationTagKey, ""),
)
}
+ registryMetadataLock.Unlock()
+
registryMetadataInfo[registryId].AddSubscribeURL(url)
}
Review Comment:
`AddService` / `AddSubscribeURL` 解锁后又无锁读取全局 map
[metadata.go:55-L57](https://github.com/apache/dubbo-go/blob/ea83f92151d134927b8620ac560b2109f716f352/metadata/metadata.go#L55),
[L68-L70](https://github.com/apache/dubbo-go/blob/ea83f92151d134927b8620ac560b2109f716f352/metadata/metadata.go#L68)
这两个函数只把 get-or-create 包在写锁里，但随后解锁，再执行
`registryMetadataInfo[registryId].AddService(url)` /
`AddSubscribeURL(url)`。这仍然是对全局 map 的无锁读；只要另一个 goroutine 正在为其他 registryId 插入
map，就可能发生读写 race。
修法建议：在锁内取出 `metadataInfo :=
registryMetadataInfo[registryId]`，解锁后只操作这个指针，不再访问全局 map。
##########
metadata/metadata_service.go:
##########
Review Comment:
`DefaultMetadataService` still races on `registryMetadataInfo`**
[metadata_service.go:98](https://github.com/apache/dubbo-go/blob/ea83f92151d134927b8620ac560b2109f716f352/metadata/metadata_service.go#L98),
[L110](https://github.com/apache/dubbo-go/blob/ea83f92151d134927b8620ac560b2109f716f352/metadata/metadata_service.go#L110),
[L128](https://github.com/apache/dubbo-go/blob/ea83f92151d134927b8620ac560b2109f716f352/metadata/metadata_service.go#L128)
PR 给 `registryMetadataInfo` 增加了 `registryMetadataLock`，但
`metadataService` 仍持有同一个 map，并在 `GetMetadataInfo` / `GetExportedServiceURLs` /
`GetSubscribedURLs` 里无锁遍历。注册或订阅并发写入 map 时，metadata service 查询仍会 race，正好是 issue
#3353 要修的场景。
我用临时 race 测试复现了：
`metadata.AddService()` 在
[metadata.go:50](https://github.com/apache/dubbo-go/blob/ea83f92151d134927b8620ac560b2109f716f352/metadata/metadata.go#L50)
写 map，同时 `DefaultMetadataService.GetExportedServiceURLs()` 在
`metadata_service.go:110` 迭代 map，race detector 报 `WARNING: DATA RACE`。
修法建议：不要让 `DefaultMetadataService` 直接遍历裸 map。可以加受锁保护的 snapshot helper，或者让
`DefaultMetadataService` 共享同一把锁并在遍历期间持 `RLock`。
--
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]
