You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "Aias00 (via GitHub)" <gi...@apache.org> on 2026/06/17 07:12:44 UTC
[PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Aias00 opened a new pull request, #3439:
URL: https://github.com/apache/dubbo-go/pull/3439
## Problem
`StickyInvoker` in `BaseClusterInvoker` was a plain `base.Invoker` field read and written by multiple goroutines without any synchronization, causing a **data race** on the RPC hot path.
### Race details
- **Write**: `DoSelect()` sets `StickyInvoker = nil` (line 104) and `StickyInvoker = selectedInvoker` (line 115)
- **Read**: `DoSelect()` reads `StickyInvoker` multiple times (lines 103, 108-110), and `IsAvailable()` reads it (lines 65-66)
- **Concurrent scenario**: `failbackClusterInvoker` calls `DoSelect()` from both the main `Invoke()` goroutine (line 143) and the retry goroutine spawned via `go invoker.tryTimerTaskProc()` (line 118, calling `DoSelect` at line 85). Even without failback, concurrent `Invoke()` calls on the same invoker would also race.
## Fix
Replace the unexported `StickyInvoker` field with `sync/atomic.Value`, accessed via `getStickyInvoker()`/`setStickyInvoker()` helpers:
| Change | Description |
|--------|-------------|
| `StickyInvoker base.Invoker` → `stickyInvoker atomic.Value` | Field uses atomic.Value, unexported to prevent direct access |
| Add `getStickyInvoker()` / `setStickyInvoker()` | Lock-free read/write wrapper methods |
| `IsAvailable()` uses `getStickyInvoker()` | Eliminates read race |
| `DoSelect()` uses local variable + atomic accessors | Eliminates read/write race; local variable ensures consistent logic within one call |
| Add `TestStickyConcurrentDoSelect` | 100 goroutines concurrently call `DoSelect` with sticky=true |
| Add `TestStickyConcurrentIsAvailableAndDoSelect` | 100 goroutines concurrently read sticky invoker while 100 goroutines call `DoSelect` |
Both new tests pass with `go test -race`.
## Semantic note
The sticky invoker mechanism is inherently "best-effort" — concurrent goroutines may select different invokers in the same call, but this is acceptable since sticky is merely a preference, not a correctness guarantee. The `atomic.Value` approach eliminates the **undefined behavior** (Go data race) while preserving the sticky semantics.
## Test results
```
$ go test ./cluster/cluster/base/... -race -count=1 -v
=== RUN TestStickyNormal
--- PASS
=== RUN TestStickyNormalWhenError
--- PASS
=== RUN TestStickyConcurrentDoSelect
--- PASS
=== RUN TestStickyConcurrentIsAvailableAndDoSelect
--- PASS
PASS
```
All other cluster package tests also pass with `-race` (the pre-existing `TestFailbackOutOfLimit` race on the `ticker` field in failback is unrelated).
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4775010728
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on code in PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#discussion_r3650392291
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -42,15 +43,15 @@ import (
type BaseClusterInvoker struct {
Directory directory.Directory
AvailableCheck bool
- Destroyed *atomic.Bool
- StickyInvoker base.Invoker
+ Destroyed *uberatomic.Bool
+ StickyInvoker atomic.Pointer[base.Invoker]
Review Comment:
[P1] 已按建议保留原签名、用内部状态修竞态：
- `StickyInvoker` 仍是导出的 `base.Invoker` 字段（未改类型）；
- `NewBaseClusterInvoker` 仍返回值 `BaseClusterInvoker`（未改为指针）；
- 各 cluster invoker 的 embed 改回值嵌入 `base.BaseClusterInvoker`；
- 竞态由 unexported `stickyLock sync.RWMutex` + `getStickyInvoker`/`setStickyInvoker` accessor 保护 `IsAvailable`/`DoSelect` 中的读写，不改变任何导出符号。
`TestStickyConcurrent*` 在 `-race` 下通过，`go vet ./cluster/...` 无 copylocks 告警。见 commit ddf000d0。
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4727900991
## [Codecov](https://app.codecov.io/gh/apache/dubbo-go/pull/3439?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 `82.35294%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 53.59%. 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 ([`d8eb2cc`](https://app.codecov.io/gh/apache/dubbo-go/commit/d8eb2cc89b7bae591a06201170509e567bc48a11?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 847 commits behind head on develop.
| [Files with missing lines](https://app.codecov.io/gh/apache/dubbo-go/pull/3439?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [cluster/cluster/base/cluster\_invoker.go](https://app.codecov.io/gh/apache/dubbo-go/pull/3439?src=pr&el=tree&filepath=cluster%2Fcluster%2Fbase%2Fcluster_invoker.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2x1c3Rlci9jbHVzdGVyL2Jhc2UvY2x1c3Rlcl9pbnZva2VyLmdv) | 82.35% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/dubbo-go/pull/3439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #3439 +/- ##
===========================================
+ Coverage 46.76% 53.59% +6.82%
===========================================
Files 295 493 +198
Lines 17172 38395 +21223
===========================================
+ Hits 8031 20578 +12547
- Misses 8287 16166 +7879
- Partials 854 1651 +797
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/dubbo-go/pull/3439?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).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4727824986
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "Alanxtl (via GitHub)" <gi...@apache.org>.
Alanxtl commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4738282896
这版把一个很小的 sticky 状态抽成了 `atomic.Value` + `stickyInvokerWrapper` + getter/setter，太java了，不太像 Go。这里其实只需要一个并发安全的指针状态，建议直接用 `atomic.Pointer` ，这里的 holder 只是为了配合 atomic，业务语义上没必要。要么直接用更轻的并发原语，要么把 holder 压到只剩一个字段，别再往上叠 getter/setter 了。
别为了适配 `atomic.Value` 再多包一层。顺手也建议把 `BaseClusterInvoker` 的使用方式收敛成只按指针持有，避免后面被值拷贝埋坑。
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4738073257
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4738334468
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4976324487
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-5078989756
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#discussion_r3426258721
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -39,7 +41,7 @@ type BaseClusterInvoker struct {
Directory directory.Directory
AvailableCheck bool
Destroyed *atomic.Bool
- StickyInvoker base.Invoker
+ stickyInvoker stdatomic.Value // stores base.Invoker
Review Comment:
`sync/atomic.Value` cannot store `nil` (it panics), so `invoker.setStickyInvoker(nil)` will crash at runtime. Also, `atomic.Value` requires all stored values have the exact same concrete dynamic type; storing different `base.Invoker` implementations over time can panic as soon as a different concrete type is stored. A robust fix is to store a single, consistent wrapper type (e.g., a struct containing `base.Invoker`) so the stored dynamic type never changes and clearing becomes storing a non-nil wrapper whose inner invoker is `nil`.
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -50,6 +52,20 @@ func NewBaseClusterInvoker(directory directory.Directory) BaseClusterInvoker {
}
}
+// getStickyInvoker returns the current sticky invoker in a race-free manner.
+func (invoker *BaseClusterInvoker) getStickyInvoker() base.Invoker {
+ v := invoker.stickyInvoker.Load()
+ if v == nil {
+ return nil
+ }
+ return v.(base.Invoker)
+}
+
+// setStickyInvoker sets the sticky invoker in a race-free manner.
+func (invoker *BaseClusterInvoker) setStickyInvoker(invokerVal base.Invoker) {
+ invoker.stickyInvoker.Store(invokerVal)
+}
Review Comment:
`sync/atomic.Value` cannot store `nil` (it panics), so `invoker.setStickyInvoker(nil)` will crash at runtime. Also, `atomic.Value` requires all stored values have the exact same concrete dynamic type; storing different `base.Invoker` implementations over time can panic as soon as a different concrete type is stored. A robust fix is to store a single, consistent wrapper type (e.g., a struct containing `base.Invoker`) so the stored dynamic type never changes and clearing becomes storing a non-nil wrapper whose inner invoker is `nil`.
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -100,19 +116,21 @@ func (invoker *BaseClusterInvoker) DoSelect(lb loadbalance.LoadBalance, invocati
// Get the service method sticky config if have
sticky = url.GetMethodParamBool(invocation.MethodName(), constant.StickyKey, sticky)
- if invoker.StickyInvoker != nil && !isInvoked(invoker.StickyInvoker, invokers) {
- invoker.StickyInvoker = nil
+ stickyInvoker := invoker.getStickyInvoker()
+ if stickyInvoker != nil && !isInvoked(stickyInvoker, invokers) {
+ invoker.setStickyInvoker(nil)
+ stickyInvoker = nil
}
if sticky && invoker.AvailableCheck &&
- invoker.StickyInvoker != nil && invoker.StickyInvoker.IsAvailable() &&
- (invoked == nil || !isInvoked(invoker.StickyInvoker, invoked)) {
- return invoker.StickyInvoker
+ stickyInvoker != nil && stickyInvoker.IsAvailable() &&
+ (invoked == nil || !isInvoked(stickyInvoker, invoked)) {
+ return stickyInvoker
}
selectedInvoker = invoker.doSelectInvoker(lb, invocation, invokers, invoked)
if sticky {
- invoker.StickyInvoker = selectedInvoker
+ invoker.setStickyInvoker(selectedInvoker)
}
Review Comment:
`sync/atomic.Value` cannot store `nil` (it panics), so `invoker.setStickyInvoker(nil)` will crash at runtime. Also, `atomic.Value` requires all stored values have the exact same concrete dynamic type; storing different `base.Invoker` implementations over time can panic as soon as a different concrete type is stored. A robust fix is to store a single, consistent wrapper type (e.g., a struct containing `base.Invoker`) so the stored dynamic type never changes and clearing becomes storing a non-nil wrapper whose inner invoker is `nil`.
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -39,7 +41,7 @@ type BaseClusterInvoker struct {
Directory directory.Directory
AvailableCheck bool
Destroyed *atomic.Bool
- StickyInvoker base.Invoker
+ stickyInvoker stdatomic.Value // stores base.Invoker
Review Comment:
This change removes/renames the previously exported `StickyInvoker` field to an unexported `stickyInvoker`, which is a breaking API change for any external package that referenced `BaseClusterInvoker.StickyInvoker`. If `BaseClusterInvoker` is part of a public API surface, consider preserving backward compatibility (e.g., keep `StickyInvoker` as a deprecated field or provide exported accessor methods and avoid removing exported fields in a non-major change).
##########
cluster/cluster/base/cluster_invoker_test.go:
##########
@@ -73,3 +74,73 @@ func TestStickyNormalWhenError(t *testing.T) {
result1 := base.DoSelect(random.NewRandomLoadBalance(), invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil), invokers, invoked)
assert.NotEqual(t, result, result1)
}
+
+// TestStickyConcurrentDoSelect verifies that concurrent calls to DoSelect
+// with sticky enabled do not cause a data race on StickyInvoker.
+func TestStickyConcurrentDoSelect(t *testing.T) {
+ var invokers []protocolbase.Invoker
+ for i := 0; i < 10; i++ {
+ url, _ := common.NewURL(fmt.Sprintf(baseClusterInvokerFormat, i))
+ url.SetParam("sticky", "true")
+ invokers = append(invokers, clusterpkg.NewMockInvoker(url, 1))
+ }
+ base := &BaseClusterInvoker{}
+ base.AvailableCheck = true
+
+ lb := random.NewRandomLoadBalance()
+ invocation1 := invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil)
+
+ const concurrency = 100
+ var wg sync.WaitGroup
+ wg.Add(concurrency)
+ for i := 0; i < concurrency; i++ {
+ go func() {
+ defer wg.Done()
+ invoked := make([]protocolbase.Invoker, 0)
+ result := base.DoSelect(lb, invocation1, invokers, invoked)
+ assert.NotNil(t, result)
+ // Second call should return the same sticky invoker
+ result2 := base.DoSelect(lb, invocation1, invokers, invoked)
+ assert.NotNil(t, result2)
+ }()
+ }
+ wg.Wait()
+}
+
+// TestStickyConcurrentIsAvailableAndDoSelect verifies that concurrent
+// IsAvailable and DoSelect calls do not cause a data race on StickyInvoker.
Review Comment:
The test name/comments claim it verifies concurrent `IsAvailable` and `DoSelect`, but the goroutine calls `base.getStickyInvoker()` directly rather than `base.IsAvailable()`. Either update the comment/name to match what’s exercised, or adjust the test to actually call `IsAvailable()` (e.g., by providing a non-nil `Directory` stub) so it matches the stated intent.
##########
cluster/cluster/base/cluster_invoker_test.go:
##########
@@ -73,3 +74,73 @@ func TestStickyNormalWhenError(t *testing.T) {
result1 := base.DoSelect(random.NewRandomLoadBalance(), invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil), invokers, invoked)
assert.NotEqual(t, result, result1)
}
+
+// TestStickyConcurrentDoSelect verifies that concurrent calls to DoSelect
+// with sticky enabled do not cause a data race on StickyInvoker.
+func TestStickyConcurrentDoSelect(t *testing.T) {
+ var invokers []protocolbase.Invoker
+ for i := 0; i < 10; i++ {
+ url, _ := common.NewURL(fmt.Sprintf(baseClusterInvokerFormat, i))
+ url.SetParam("sticky", "true")
+ invokers = append(invokers, clusterpkg.NewMockInvoker(url, 1))
+ }
+ base := &BaseClusterInvoker{}
+ base.AvailableCheck = true
+
+ lb := random.NewRandomLoadBalance()
+ invocation1 := invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil)
+
+ const concurrency = 100
+ var wg sync.WaitGroup
+ wg.Add(concurrency)
+ for i := 0; i < concurrency; i++ {
+ go func() {
+ defer wg.Done()
+ invoked := make([]protocolbase.Invoker, 0)
+ result := base.DoSelect(lb, invocation1, invokers, invoked)
+ assert.NotNil(t, result)
+ // Second call should return the same sticky invoker
+ result2 := base.DoSelect(lb, invocation1, invokers, invoked)
+ assert.NotNil(t, result2)
Review Comment:
The test comment states the second call should return the same sticky invoker, but the test doesn’t assert that invariant. Consider asserting `result == result2` (or equivalent) so the test verifies sticky semantics in addition to relying on `-race`.
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -100,19 +116,21 @@ func (invoker *BaseClusterInvoker) DoSelect(lb loadbalance.LoadBalance, invocati
// Get the service method sticky config if have
sticky = url.GetMethodParamBool(invocation.MethodName(), constant.StickyKey, sticky)
- if invoker.StickyInvoker != nil && !isInvoked(invoker.StickyInvoker, invokers) {
- invoker.StickyInvoker = nil
+ stickyInvoker := invoker.getStickyInvoker()
+ if stickyInvoker != nil && !isInvoked(stickyInvoker, invokers) {
+ invoker.setStickyInvoker(nil)
+ stickyInvoker = nil
}
Review Comment:
`sync/atomic.Value` cannot store `nil` (it panics), so `invoker.setStickyInvoker(nil)` will crash at runtime. Also, `atomic.Value` requires all stored values have the exact same concrete dynamic type; storing different `base.Invoker` implementations over time can panic as soon as a different concrete type is stored. A robust fix is to store a single, consistent wrapper type (e.g., a struct containing `base.Invoker`) so the stored dynamic type never changes and clearing becomes storing a non-nil wrapper whose inner invoker is `nil`.
##########
cluster/cluster/base/cluster_invoker_test.go:
##########
@@ -73,3 +74,73 @@ func TestStickyNormalWhenError(t *testing.T) {
result1 := base.DoSelect(random.NewRandomLoadBalance(), invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil), invokers, invoked)
assert.NotEqual(t, result, result1)
}
+
+// TestStickyConcurrentDoSelect verifies that concurrent calls to DoSelect
+// with sticky enabled do not cause a data race on StickyInvoker.
+func TestStickyConcurrentDoSelect(t *testing.T) {
+ var invokers []protocolbase.Invoker
+ for i := 0; i < 10; i++ {
+ url, _ := common.NewURL(fmt.Sprintf(baseClusterInvokerFormat, i))
+ url.SetParam("sticky", "true")
+ invokers = append(invokers, clusterpkg.NewMockInvoker(url, 1))
+ }
+ base := &BaseClusterInvoker{}
+ base.AvailableCheck = true
+
+ lb := random.NewRandomLoadBalance()
+ invocation1 := invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil)
+
+ const concurrency = 100
+ var wg sync.WaitGroup
+ wg.Add(concurrency)
+ for i := 0; i < concurrency; i++ {
+ go func() {
+ defer wg.Done()
+ invoked := make([]protocolbase.Invoker, 0)
+ result := base.DoSelect(lb, invocation1, invokers, invoked)
+ assert.NotNil(t, result)
+ // Second call should return the same sticky invoker
+ result2 := base.DoSelect(lb, invocation1, invokers, invoked)
+ assert.NotNil(t, result2)
+ }()
+ }
+ wg.Wait()
+}
+
+// TestStickyConcurrentIsAvailableAndDoSelect verifies that concurrent
+// IsAvailable and DoSelect calls do not cause a data race on StickyInvoker.
+// Only the sticky invoker path is exercised (not the Directory fallback),
+// since Directory is nil in this test setup.
+func TestStickyConcurrentIsAvailableAndDoSelect(t *testing.T) {
+ var invokers []protocolbase.Invoker
+ for i := 0; i < 10; i++ {
+ url, _ := common.NewURL(fmt.Sprintf(baseClusterInvokerFormat, i))
+ url.SetParam("sticky", "true")
+ invokers = append(invokers, clusterpkg.NewMockInvoker(url, 1))
+ }
+ base := &BaseClusterInvoker{}
+ base.AvailableCheck = true
+
+ lb := random.NewRandomLoadBalance()
+ invocation1 := invocation.NewRPCInvocation(baseClusterInvokerMethodName, nil, nil)
+
+ // First DoSelect to set the sticky invoker so IsAvailable uses the sticky path
+ invoked := make([]protocolbase.Invoker, 0)
+ base.DoSelect(lb, invocation1, invokers, invoked)
+
+ const concurrency = 100
+ var wg sync.WaitGroup
+ wg.Add(concurrency * 2)
+ for i := 0; i < concurrency; i++ {
+ go func() {
+ defer wg.Done()
+ // IsAvailable reads StickyInvoker; this would race without atomic protection
+ _ = base.getStickyInvoker()
Review Comment:
The test name/comments claim it verifies concurrent `IsAvailable` and `DoSelect`, but the goroutine calls `base.getStickyInvoker()` directly rather than `base.IsAvailable()`. Either update the comment/name to match what’s exercised, or adjust the test to actually call `IsAvailable()` (e.g., by providing a non-nil `Directory` stub) so it matches the stated intent.
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4727090095
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4727598307
## Addressing Copilot Review Feedback
Thanks for the thorough review! All three issues have been fixed in the latest push:
### 1. `atomic.Value.Store(nil)` panic ✅ Fixed
**Problem:** `atomic.Value.Store(nil)` panics at runtime.
**Fix:** Introduced `stickyInvokerWrapper` struct to wrap `base.Invoker`:
```go
type stickyInvokerWrapper struct {
invoker base.Invoker
}
```
Now clearing the sticky invoker stores `stickyInvokerWrapper{invoker: nil}` (non-nil wrapper with nil inner value), which is safe.
### 2. Inconsistent concrete type panic ✅ Fixed
**Problem:** `atomic.Value` requires all stored values to have the same concrete dynamic type. Different `base.Invoker` implementations would cause a panic.
**Fix:** The `stickyInvokerWrapper` type is always the same regardless of the concrete `base.Invoker` implementation stored inside.
### 3. Test calling `getStickyInvoker()` instead of `IsAvailable()` ✅ Fixed
**Problem:** `TestStickyConcurrentIsAvailableAndDoSelect` called the unexported `getStickyInvoker()` instead of the public API `IsAvailable()`.
**Fix:** Test now uses `NewBaseClusterInvoker()` with a proper `mockDirectory` implementation so `IsAvailable()` works end-to-end without panicking on nil Directory.
### 4. Import formatting ✅ Fixed
Ran `make fmt` (imports-formatter) to match project convention (separate import groups for stdlib, third-party, and internal packages).
### CI Notes
- **Check Code Format**: Should pass now after running `make fmt`
- **Integration Test failure**: Pre-existing infrastructure timeout, not related to this change
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks merged PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4727086492
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4727601667
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=3439&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=3439&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=3439&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=3439)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#issuecomment-4738297038
> 这版把一个很小的 sticky 状态抽成了 `atomic.Value` + `stickyInvokerWrapper` + getter/setter，太java了，不太像 Go。这里其实只需要一个并发安全的指针状态，建议直接用 `atomic.Pointer` ，这里的 holder 只是为了配合 atomic，业务语义上没必要。要么直接用更轻的并发原语，要么把 holder 压到只剩一个字段，别再往上叠 getter/setter 了。
>
> 别为了适配 `atomic.Value` 再多包一层。顺手也建议把 `BaseClusterInvoker` 的使用方式收敛成只按指针持有，避免后面被值拷贝埋坑。
好 我处理下
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(cluster): eliminate data race on StickyInvoker with atomic.Value [dubbo-go]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on code in PR #3439:
URL: https://github.com/apache/dubbo-go/pull/3439#discussion_r3610089405
##########
cluster/cluster/base/cluster_invoker.go:
##########
@@ -42,15 +43,15 @@ import (
type BaseClusterInvoker struct {
Directory directory.Directory
AvailableCheck bool
- Destroyed *atomic.Bool
- StickyInvoker base.Invoker
+ Destroyed *uberatomic.Bool
+ StickyInvoker atomic.Pointer[base.Invoker]
Review Comment:
[P1] 不要在 v3 补丁版本中改变已发布的导出 API
`BaseClusterInvoker.StickyInvoker` 和 `NewBaseClusterInvoker` 已随 `v3.3.2-20260709` 发布。外部消费者在该版本可将 `StickyInvoker` 作为 `base.Invoker` 使用，并把构造函数结果赋给 `BaseClusterInvoker` 值；同一探针替换为当前 Head 后分别因字段变成 `atomic.Pointer[base.Invoker]`、构造函数变成 `*BaseClusterInvoker` 而编译失败。现有旧评论针对的是更早的“字段改为未导出”实现，当前类型变化仍是独立的源码兼容性破坏。建议在 v3 保留原签名并通过不改变导出符号的内部状态实现竞态修复，或将该 breaking change 延后到下一个 major，并补外部消费者编译门禁。
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org