[I] [BUG] cluster/failback: process goroutine leak on Destroy, ticker/taskList/retryTask races, nil-Dispose panic [dubbo-go]

["Aias00 (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@dubbo.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

Re: [I] [BUG] cluster/failback: process goroutine leak on Destroy, ticker/taskList/retryTask races, nil-Dispose panic [dubbo-go]

["Alanxtl (via GitHub)" <gi...@apache.org>]

Alanxtl closed issue #3526: [BUG] cluster/failback: process goroutine leak on Destroy, ticker/taskList/retryTask races, nil-Dispose panic
URL: https://github.com/apache/dubbo-go/issues/3526


-- 
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: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

Re: [I] [BUG] cluster/failback: process goroutine leak on Destroy, ticker/taskList/retryTask races, nil-Dispose panic [dubbo-go]

["Alanxtl (via GitHub)" <gi...@apache.org>]

Alanxtl commented on issue #3526:
URL: https://github.com/apache/dubbo-go/issues/3526#issuecomment-5086270072

   not planned


-- 
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: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org