Aias00 opened a new issue, #3526:
URL: https://github.com/apache/dubbo-go/issues/3526
### Problem
`failbackClusterInvoker` has three defects in its retry lifecycle, all
surfaced by `go test -race` (the `cluster/cluster/failback` package fails
`TestFailbackRetryFailed` and `TestFailbackOutOfLimit` under `-race` on
`develop`):
1. **Goroutine leak.** `process` drives the retry loop with `for range
invoker.ticker.C`. `Destroy` calls `ticker.Stop()`, but `time.Ticker.Stop()`
does **not** close `ticker.C`, so the goroutine blocks on `range .C` forever
waiting for a tick that never comes. The loop body's `Peek → ErrDisposed →
return` exit is unreachable because the body only runs on a tick. Every
failback invoker that ever failed a call leaks one goroutine + ticker for the
life of the process (compounds across registry refresh / provider reconnect
cycles).
2. **`ticker` field race.** `invoker.ticker` is written from the `process`
goroutine and read in `Destroy` with no synchronization.
3. **`taskList` nil-pointer on Destroy.** `taskList` is initialized lazily
(only when an `Invoke` fails). If `Destroy` is called on an invoker whose every
`Invoke` succeeded (or was never called), `taskList` is nil and
`invoker.taskList.Dispose()` dereferences a nil `*queue.Queue`.
4. **`retryTimerTask.lastT` field race.** `checkRetry` writes `t.lastT =
time.Now()` **after** `taskList.Put(t)`, while `process` reads
`retryTask.lastT` after `Peek`. The queue's Put/Peek synchronization only makes
**pre-Put** writes visible to Peek, so the post-Put `lastT` write races
process's read.
### Current behavior
```go
// process — leaks, and reads ticker/taskList fields
func (invoker *failbackClusterInvoker) process(ctx context.Context) {
invoker.ticker = time.NewTicker(time.Second * 1) // unsync write
for range invoker.ticker.C { // blocks forever
after Stop
for { ... invoker.taskList.Peek() ... retryTask.lastT ... } //
reads lastT
}
}
// Destroy — reads ticker (race), nil-derefs taskList, can't stop process
func (invoker *failbackClusterInvoker) Destroy() {
invoker.BaseClusterInvoker.Destroy()
if invoker.ticker != nil { invoker.ticker.Stop() } // unsync read;
Stop doesn't close .C
_ = invoker.taskList.Dispose() // nil-deref if
never failed
}
// checkRetry — writes lastT after Put (race)
func (t *retryTimerTask) checkRetry() {
...
t.clusterInvoker.taskList.Put(t)
t.lastT = time.Now() // post-Put write races process's Peek-read of
lastT
}
```
### Expected behavior
1. `Destroy` must deterministically stop the `process` goroutine (no leak).
2. `ticker`/`taskList`/lifecycle fields must be accessed under a lock so
`Destroy` observes a consistent state.
3. `Destroy` on a never-failed invoker must not panic.
4. `retryTimerTask` field writes must be visible to `process`'s Peek-read
(happen-before via the queue).
### Suggested approach
- Replace the `once sync.Once` lazy init with a `mu sync.Mutex` +
`initialized bool` `ensureInit`, so `Destroy` can take the same lock and
observe the state.
- Add a `done chan struct{}`; `process` selects on `ticker.C` **and**
`done`, with a `defer ticker.Stop()`; `Destroy` closes `done`.
- `Destroy` nil-checks `taskList` (only Dispose if initialized).
- In `checkRetry`, set `t.lastT` **before** `taskList.Put(t)` so the write
is published to the queue and visible to Peek.
### Acceptance criteria
- [ ] `Destroy` stops the `process` goroutine (no goroutine leak across
Subscribe/Destroy cycles).
- [ ] `Destroy` on a never-failed failback invoker does not panic.
- [ ] `ticker`/`taskList`/`done` access is synchronized; no data race.
- [ ] `checkRetry` writes `lastT` before `Put`.
- [ ] `cluster/cluster/failback` passes under `-race`, including
`TestFailbackRetryFailed` and `TestFailbackOutOfLimit` (both fail on `develop`).
--
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]
