Aias00 opened a new issue, #3528:
URL: https://github.com/apache/dubbo-go/issues/3528
### Problem
`ServiceInstancesChangedListenerImpl.OnEvent` holds `lstn.mutex` across the
entire method and dispatches to consumer `NotifyListener`s **under that lock**:
```go
// registry/servicediscovery/service_instances_changed_listener_impl.go
func (lstn *ServiceInstancesChangedListenerImpl) OnEvent(e observer.Event)
error {
...
lstn.mutex.Lock()
defer lstn.mutex.Unlock()
// ... rebuild allInstances / revisionToMetadata / serviceUrls ...
for key, notifyListener := range lstn.listeners {
...
notifyListener.NotifyAll(events, func() {}) // external callback
UNDER lock
}
return nil
}
```
`AddListenerAndNotify` and `RemoveListener` both acquire `lstn.mutex`. Two
consequences:
1. **Self-deadlock.** `sync.Mutex` is not reentrant. If a consumer's
`NotifyAll` synchronously re-enters `AddListenerAndNotify`/`RemoveListener` on
the *same* `ServiceInstancesChangedListenerImpl` (e.g. a directory
re-subscribing during notification), the re-entrant `Lock` blocks forever — the
OnEvent goroutine holds the mutex and waits for itself. (The same listener
instance is the natural re-entry target because `NotifyAll` is the notification
path and `AddListenerAndNotify`/`RemoveListener` is the subscribe/unsubscribe
path on the same object.)
2. **Tail latency.** Holding the mutex across the external callback means
any consumer that blocks in `NotifyAll` (URL merge, downstream RPC, lock
contention) stalls **every other** `OnEvent` for that listener, serializing all
instance-change processing for the application.
This is the same class of issue the reviewer flagged on PR #3442 (do not
hold a registry lock across external calls). The sibling method
`AddListenerAndNotify` already snapshots under the lock and notifies outside
it; `OnEvent` does not.
### Current behavior
- `OnEvent` holds `lstn.mutex` from the top of the method through the
`NotifyAll` dispatch loop.
- A `NotifyAll` callback that re-enters
`AddListenerAndNotify`/`RemoveListener` on the same listener self-deadlocks.
- A slow/blocking `NotifyAll` blocks all other `OnEvent` calls.
### Expected behavior
`OnEvent` should build the per-listener event batches under the lock,
release the lock, and dispatch `NotifyAll` outside the lock — matching
`AddListenerAndNotify`.
### Suggested approach
- Snapshot `(notifyListener, events)` pairs under `lstn.mutex`, then
explicitly `Unlock()` before the dispatch loop.
- Verify there is no early `return` between the `Lock` and the explicit
`Unlock` that would leak the mutex (there is not — the build loop uses
`continue`, not `return`).
### Acceptance criteria
- [ ] `NotifyAll` is dispatched outside `lstn.mutex`.
- [ ] A `NotifyAll` callback that re-enters
`AddListenerAndNotify`/`RemoveListener` on the same listener no longer
deadlocks (regression test).
- [ ] Existing `servicediscovery` tests remain green 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]
