Aias00 opened a new issue, #3542:
URL: https://github.com/apache/dubbo-go/issues/3542
### Problem
`apolloListener.listeners` is a plain `map[ConfigurationListener]struct{}`
with no mutex:
```go
// config_center/apollo/listener.go
type apolloListener struct {
listeners map[config_center.ConfigurationListener]struct{}
}
func (a *apolloListener) OnNewestChange(changeEvent
*storage.FullChangeEvent) {
...
for listener := range a.listeners { // ranged on agollo's long-poll
goroutine
listener.Process(...)
}
}
func (a *apolloListener) AddListener(l config_center.ConfigurationListener) {
if _, ok := a.listeners[l]; !ok { a.listeners[l] = struct{}{} } //
router goroutine
}
func (a *apolloListener) RemoveListener(l
config_center.ConfigurationListener) {
delete(a.listeners, l) // router goroutine
}
func (a *apolloListener) IsEmpty() bool { return len(a.listeners) == 0 }
// also unlocked
```
`OnNewestChange` runs on agollo's long-poll callback goroutine while
`AddListener`/`RemoveListener` run on dubbo's router goroutines (via
`apolloConfiguration.AddListener`/`RemoveListener`, `impl.go:85-107`).
Concurrent map read+write is a fatal Go runtime error (`fatal error: concurrent
map read and map write`), not just a benign race. `IsEmpty` reads the same map
unlocked.
This is the same class of bug fixed in the zk config_center `CacheListener`
(#3539/#3540); apollo's `apolloListener` has the identical pattern with no lock.
### Current behavior
A config push from Apollo arriving while a router subscribes/unsubscribes
(or `IsEmpty` is called by `RemoveListener` deciding whether to
`RemoveChangeListener`) crashes the process with a fatal concurrent-map-access
error, or silently corrupts the listener map (dropping/duplicating callbacks).
### Expected behavior
The listener set must be guarded by a mutex, and `OnNewestChange` must
dispatch `Process` outside the lock so a slow consumer cannot block agollo's
long-poll goroutine.
### Suggested approach
- Add a `sync.RWMutex` to `apolloListener`.
- `AddListener`/`RemoveListener` take the write lock; `IsEmpty` takes the
read lock.
- `OnNewestChange` snapshots the listeners under the read lock and
dispatches `Process` outside it.
### Acceptance criteria
- [ ] `apolloListener.listeners` is accessed under the mutex in all methods.
- [ ] `OnNewestChange` dispatches `Process` outside the lock.
- [ ] A concurrency test (concurrent `OnNewestChange` vs `Add`/`Remove`)
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]
