fix: close #3247 config/protocol shared-state races#3271
fix: close #3247 config/protocol shared-state races#3271xxs588 wants to merge 5 commits intoapache:developfrom
Conversation
- guard global rootConfig pointer with RWMutex-backed accessors - make config.Load build a local RootConfig and initialize before publishing - route config consumers through GetRootConfig instead of direct global reads - avoid in-place global mutation in SetConsumerConfig - add concurrent Set/Get regression test for root config Fixes: apache#3247
- remove package-level versionInt maps in dubbo impl and hessian2 response paths - compute version int per call to avoid shared mutable/global state - add concurrent isSupportResponseAttachment tests in both packages Fixes: apache#3247
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3271 +/- ##
===========================================
+ Coverage 46.76% 48.24% +1.48%
===========================================
Files 295 466 +171
Lines 17172 34078 +16906
===========================================
+ Hits 8031 16442 +8411
- Misses 8287 16305 +8018
- Partials 854 1331 +477 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xxs588
force-pushed
the
split/3247-config-protocol
branch
from
9a614b1 to
6fb9abe
Compare
xxs588
changed the title
| func getRootConfigInternal() *RootConfig { | ||
| rootConfigMu.RLock() | ||
| defer rootConfigMu.RUnlock() | ||
| return rootConfig | ||
| } |
There was a problem hiding this comment.
已经修改了，把加锁逻辑去掉了，换成原子读写rootConfig ，然后修改了相关的单测，将直接读全局变量的地方改成统一走 GetRootConfig()/SetRootConfig()
AlexStocks
left a comment
AlexStocks
left a comment
There was a problem hiding this comment.
修复思路清晰，mutex 保护 rootConfig 的方向没问题，并发测试也补得比较完整。有两个小问题，详见行内评论。
| @@ -185,7 +185,14 @@ func (cc *ConsumerConfig) Load() { | |||
|
|
|||
| // SetConsumerConfig sets consumerConfig by @c | |||
| func SetConsumerConfig(c ConsumerConfig) { | |||
There was a problem hiding this comment.
SetConsumerConfig 里的 nil 分支在 rc == nil 时会丢失其他字段。如果 GetRootConfig() 还没被调用过（返回 nil），这里直接 SetRootConfig(RootConfig{Consumer: &c}) 会创建一个只有 Consumer 的 RootConfig，之前通过 Load() 设置的 Protocol、Application 等字段全部丢失。
建议：如果 GetRootConfig() 返回 nil，直接用 NewRootConfigBuilder() 构建一个默认值，再设置 Consumer：
rc := GetRootConfig()
if rc == nil {
rc = NewRootConfigBuilder().Build()
}
next := *rc
next.Consumer = &c
SetRootConfig(next)
| @@ -90,3 +92,24 @@ func TestNewRootConfigBuilder(t *testing.T) { | |||
| config := GetRootConfig() | |||
| assert.Equal(t, rootConfig, config) | |||
| } | |||
There was a problem hiding this comment.
TestRootConfigConcurrentSetAndGet 的 200 个并发 goroutine 数量在某些低端 CI 环境（如树莓派或低配虚拟机）上可能会超时或触发 race detector。建议把数量降低到 50，或者加上 go test 的 -p 限制。
另外，这个测试只验证了不 panic 和 GetRootConfig() 非 nil，建议加一个 assert 检查在所有 Set/Get 完成后，GetRootConfig() 返回的是一个有效的 RootConfig（Application.Name 不为空）。
|
4 participants
Description
Fixes #3247
config中为rootConfig增加受控读写路径（GetRootConfig/SetRootConfig），并将Load调整为“本地构建并初始化后再发布”protocol/dubbo中移除包级共享versionIntmap，isSupportResponseAttachment改为按次调用version2Int计算TestRootConfigConcurrentSetAndGet、TestIsSupportResponseAttachmentConcurrent（impl/hessian2）Verification
Checklist
develop