Aias00 opened a new issue, #3512:
URL: https://github.com/apache/dubbo-go/issues/3512
### Problem
In the Zookeeper / etcdv3 / Nacos service-discovery backends, the watcher /
subscription callback (`DataChange` or the Nacos `SubscribeCallback`) reads
`instanceListenerMap[serviceName]` **without holding the lock** that protects
it, then calls `.Values()` on the returned `*gxset.HashSet`.
- `instanceListenerMap` is written under a lock (`listenLock` /
`listenerLock` / the package-global `initLock`) in `AddListener` /
`registerInstanceListener` / `Destroy`, but the callback reads it unlocked →
concurrent map read/write, which is a fatal `fatal error: concurrent map read
and map write` in Go.
- When the map has no entry for `serviceName` (stale watch, a service
registered but never subscribed, or after `Destroy` cleared the map),
`instanceListenerMap[serviceName]` returns a nil `*gxset.HashSet`; `.Values()`
on a nil receiver dereferences it → process panic.
The etcdv3 case is deterministic: `DataChange` sets `instance.ServiceName =
""` when JSON decoding fails, then looks up `instanceListenerMap[""]` (almost
certainly nil) → guaranteed panic on any malformed etcd value.
### Current behavior
`registry/zookeeper/service_discovery.go` (`DataChange`, ~line 279)
```go
instances := zksd.GetInstances(serviceName)
for _, lis := range zksd.instanceListenerMap[serviceName].Values() { // no
listenLock; nil .Values() panic
instanceListener := lis.(registry.ServiceInstancesChangedListener)
err =
instanceListener.OnEvent(registry.NewServiceInstancesChangedEvent(serviceName,
instances))
}
```
`registry/etcdv3/service_discovery.go` (`DataChange`, ~lines 284-292)
```go
err := jsonutil.DecodeJSON([]byte(eventType.Content), &instance)
if err != nil {
instance.ServiceName = "" // decode failure → guaranteed nil
lookup
}
name := instance.ServiceName
instances := e.GetInstances(name)
for _, lis := range e.instanceListenerMap[instance.ServiceName].Values() {
// no initLock; nil panic
instanceLis := lis.(registry.ServiceInstancesChangedListener)
err = instanceLis.OnEvent(registry.NewServiceInstancesChangedEvent(name,
instances))
}
```
`registry/nacos/service_discovery.go` (`SubscribeCallback`, ~line 320)
```go
for _, lis := range n.instanceListenerMap[serviceName].Values() { // no
listenerLock; nil panic after Destroy
...
}
```
### Expected behavior
1. Reads of `instanceListenerMap` in callbacks must hold the same lock used
by writers.
2. A missing map entry must not crash — a watcher firing for a service with
no listeners should be a no-op (possibly logged).
3. etcdv3 must not blank `ServiceName` to `""` and then look it up; decode
failure should drop the event with a warning.
### Suggested approach
- Take the corresponding lock around the map read in each `DataChange` /
callback, or snapshot the listener set under the lock and iterate the snapshot.
- Guard every `m[name].Values()` with `if set, ok := m[name]; ok && set !=
nil { ... }`.
- etcdv3: on decode error, `return` (with a warning) instead of setting
`ServiceName = ""`.
- Add concurrency tests: watcher firing for an un-subscribed service name;
callback delivered after `Destroy`.
### Acceptance criteria
- [ ] zk / etcdv3 / nacos callback paths read `instanceListenerMap` under
the writer lock and guard nil.
- [ ] A watcher event for a service with no listeners no longer panics.
- [ ] etcdv3 no longer panics on an undecodable value; it warns and skips.
- [ ] `Destroy` racing an in-flight callback no longer triggers a fatal map
race (`-race` clean).
--
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]
