AlexStocks commented on code in PR #3269:
URL: https://github.com/apache/dubbo-go/pull/3269#discussion_r2969115687
##########
filter/hystrix/filter.go:
##########
@@ -162,6 +162,7 @@ func (f *Filter) Invoke(ctx context.Context, invoker
base.Invoker, invocation ba
configLoadMutex.Unlock()
}
configLoadMutex.RLock()
+ methodRegexps := append([]*regexp.Regexp(nil),
f.res[invocation.MethodName()]...)
Review Comment:
这里在 RUnlock 之前做了 map copy，然后在 Do 的回调里用 copy 没问题（避免了迭代时的 race）。但 `hystrix.Do`
的回调是异步执行的，hystrix 内部维护的 circuit 状态在 Do 返回后仍可能变化，和 filter 层的 goroutine 安全是两回事。
这个是原有设计问题，不是这次引入的。hystrix filter 本身整体是 non-thread-safe 的，注释里应该加一句说明这个 filter
不支持并发调用，或者考虑在 Invoke 入口加锁。
##########
filter/hystrix/filter.go:
##########
@@ -162,6 +162,7 @@ func (f *Filter) Invoke(ctx context.Context, invoker
base.Invoker, invocation ba
configLoadMutex.Unlock()
}
configLoadMutex.RLock()
+ methodRegexps := append([]*regexp.Regexp(nil),
f.res[invocation.MethodName()]...)
Review Comment:
copy 语义正确，但建议明确注释说明这是在释放锁之前做 copy，避免后续维护者误以为可以在锁外再做 copy：
```go
// Must copy before unlock to avoid concurrent map read/write during
iteration
methodRegexps := append([]*regexp.Regexp(nil),
f.res[invocation.MethodName()]...)
```
##########
config_center/file/listener.go:
##########
@@ -151,12 +187,9 @@ func (cl *CacheListener) Close() error {
func (cl *CacheListener) AddListener(key string, listener
config_center.ConfigurationListener) {
Review Comment:
listenerSet.Snapshot() 在 RLock 里面做了 copy，但返回的 []ConfigurationListener
里每个元素是指针。如果外部对这些 listener 调用了非线程安全的操作，仍然会有问题。不过这是调用方的问题，不是 snapshot 本身的问题。
建议在 Snapshot() 的注释里说明返回的是只读快照，调用方不应修改 snapshot 里的 listener。
##########
metrics/common.go:
##########
@@ -43,19 +47,25 @@ type ApplicationMetricLevel struct {
HostName string
}
-var applicationName string
-var applicationVersion string
+var (
+ appInfoLock sync.RWMutex
+ applicationName string
+ applicationVersion string
+)
Review Comment:
InitAppInfo 写全局变量有锁，但 NewConfigCenterLevel 里调 getAppInfo() 是只读没问题。不过
InitAppInfo 只在应用启动时调用一次（正常路径），而 GetApplicationLevel 在每次上报 metric 时都可能调用，这里用
RLock 是对的。
有个潜在问题：如果 InitAppInfo 写入了不完整的状态（比如只写了 name 没写 version），GetApplicationLevel
读到的是新 name + 旧 version 的组合。不过这是逻辑问题，不是 race 问题。建议在 InitAppInfo
里把两个赋值放在一次锁里面，已经是这样了，没问题。
--
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]
