Aias00 opened a new issue, #3522:
URL: https://github.com/apache/dubbo-go/issues/3522
### Problem
Both `cluster/loadbalance/p2c` and `cluster/cluster/adaptivesvc` initialize
a package-level singleton with the same buggy pattern: an `if instance == nil`
check **outside** `sync.Once.Do`, then `return instance` also outside `Do`.
```go
var (
once sync.Once
instance loadbalance.LoadBalance
)
func newP2CLoadBalance(r randomPicker) loadbalance.LoadBalance {
if r == nil { r = defaultRnd }
if instance == nil { // unsynchronized read
once.Do(func() { instance = &p2cLoadBalance{...} })
}
return instance // unsynchronized read when the check
was false
}
```
`sync.Once.Do` only establishes a happens-before for callers that **call
`Do`**. A goroutine that observes `instance != nil` (check false) skips `Do`
entirely and reads `instance` with no happens-before to the goroutine that
wrote it inside `Do`. Two concurrent first-time callers therefore race on
`instance`: `go test -race` flags it. The identical pattern exists in
`cluster/cluster/adaptivesvc/cluster.go:newAdaptiveServiceCluster`.
Because both factories are registered in `init()` via
`extension.SetLoadbalance`/`SetCluster`, every concurrent RPC at process start
(before first use) hits this path.
### Current behavior
- `cluster/loadbalance/p2c/loadbalance.go` `newP2CLoadBalance` reads
`instance` outside `once.Do`.
- `cluster/cluster/adaptivesvc/cluster.go` `newAdaptiveServiceCluster` reads
`instance` outside `once.Do`.
### Expected behavior
The read of `instance` must have a happens-before relation to the write, so
concurrent first-time callers cannot race.
### Suggested approach
- Drop the `if instance == nil` fast path; always call `once.Do(...)` then
`return instance`. `sync.Once.Do` guarantees all callers see the writes done
inside `Do`, and there is no read of `instance` outside `Do`.
- The `r == nil` defaulting in p2c moves inside the `once.Do` closure (only
the first call's `r` is used, identical to before).
### Acceptance criteria
- [ ] No read of `instance` outside `once.Do` in either package.
- [ ] A concurrency test (100 goroutines racing the factory) passes under
`-race`.
--
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]
