AlexStocks commented on code in PR #993:
URL: https://github.com/apache/dubbo-go-pixiu/pull/993#discussion_r4180466699
##########
pkg/server/listener_manager.go:
##########
@@ -117,31 +117,136 @@ func (lm *ListenerManager) gracefulShutdownInit() {
sig := <-signals
logger.Infof("get signal %s, dubbo-go-pixiu will start
shutdown.", sig)
- time.AfterFunc(timeout, func() {
- logger.Warn("Shutdown gracefully timeout, listeners
will shutdown immediately. ")
- os.Exit(0)
+ shutdownErrors, timedOut := lm.handleShutdownSignal(sig,
timeout)
+
+ if len(shutdownErrors) > 0 {
+ logger.Errorf("Shutdown completed with %d errors",
len(shutdownErrors))
+ }
+ if timedOut {
+ logger.Warn("Shutdown gracefully timeout, some
listeners may not have shut down cleanly")
+ }
+ os.Exit(shutdownExitCode(shutdownErrors, timedOut))
+ }()
+}
+
+func shutdownExitCode(shutdownErrors []error, timedOut bool) int {
+ if timedOut || len(shutdownErrors) > 0 {
+ return 1
+ }
+ return 0
+}
+
+// handleShutdownSignal processes a shutdown signal by coordinating listener
shutdowns.
+// It returns a slice of errors from failed shutdowns and a boolean indicating
timeout.
+// This function is extracted from gracefulShutdownInit for testability.
+func (lm *ListenerManager) handleShutdownSignal(sig os.Signal, timeout
time.Duration) ([]error, bool) {
+ lm.rwLock.RLock()
+ listeners := make([]*wrapListenerService, 0,
len(lm.activeListenerService))
+ for _, listener := range lm.activeListenerService {
+ listeners = append(listeners, listener)
+ }
+ lm.rwLock.RUnlock()
+
+ shutdownWG := &sync.WaitGroup{}
+ shutdownWG.Add(len(listeners))
Review Comment:
[P2] `shutdownWG` 现在是死代码，而且它的预加计数已经和 listener 契约脱钩。
这个 WaitGroup 现在只有两个用途：`Add(len(listeners))`，以及作为参数传给
`listener.ShutDown(shutdownWG)`。全仓搜下来，**没有任何地方再 `Wait()`
它**（`listener_manager.go:150-155` 是全部出现点）。协调改由 `doneCh`/`allDone`
承担，所以计数失衡当下不会造成阻塞。
但这留下一个真实的契约问题。`pkg/listener/listener.go:44` 的接口签名是 `ShutDown(any)
error`，各内置实现都在函数内部自己 `wg.(*sync.WaitGroup).Done()`，而 base 的写法是"调用方
`Add(1)`、listener 自己 `Done()`"——计数配对是 listener 的责任。本 PR 把 `Add`
挪到了外部一次性预加，语义就变了。
具体到内置实现：`tcp_listener.go:117-119`、`http_listener.go:112-114`、`triple_listener.go:113-115`、`http2_listener.go:164-166`
都有同一个形状的早退：
```go
timeout := config.GetBootstrap().GetShutdownConfig().GetTimeout()
if timeout <= 0 {
return nil // <- Done() 不可达
}
defer wg.(*sync.WaitGroup).Done()
```
`shutdownWG.Add(len(listeners))` 预加了 N，但这些早退路径一个 `Done()`
都不会调。我写了个探针复刻这个形状，确认 `wg.Wait()`
永远不会返回（存证：`evidence/probe-wg-leak.txt`，`TestProductionWaitGroupLeaks` FAIL /
`TestDoneChStillCloses` PASS）。
**为什么定为 P2 而不是 P0**：这条路径在当前内置 listener 上**不可达**。`gracefulShutdownInit`
自己有一道守卫（`listener_manager.go:108-111`：`timeout <= 0` 直接 return，不注册 signal
handler），所以 `handleShutdownSignal` 只在 `timeout > 0` 时被调用；而各 listener 读的是同一个
`config.GetBootstrap().GetShutdownConfig().GetTimeout()`，此时必然 `> 0`，早退不会触发。探针里
`TestDoneChStillCloses` 通过也印证了新的 `doneCh` 路径本身是对的。
但对外的 `ListenerService` 接口是给插件用的，第三方 listener 完全可能实现成"timeout<=0 就直接返回"（这在
base 里是安全的，因为 base 根本不 `Wait()`——超时由 `time.AfterFunc` 兜底）。合并后这份代码会以 N 个永不
`Done()` 的计数交付给插件作者，而注释里"deliberately separate ... keep existing out-of-tree
listener plugins compiling"明确表达了要照顾外部插件。
建议二选一：
- 如果确认 `doneCh` 已经是唯一的协调机制，就把 `shutdownWG` 整个删掉，`ShutDown(nil)` 传 nil，同时在
`ListenerService` 接口注释里写明"不再要求实现方调用 `Done()`"——但这是 breaking change，需要单独说明；
- 或保持 `Add`/`Done` 配对，把 `Add(1)` 交还给每个 listener（即 `go func(){
shutdownWG.Add(1); defer shutdownWG.Done(); ... }()` 之前的原始写法），`shutdownWG`
仅作为插件可见的信号保留，并修掉内置实现里 `timeout<=0` 的早退（`defer wg.Done()` 提到 `timeout` 判断之前）。
另外 Copilot 那条关于 `errCh` 顺序的意见我核实后不成立：`doneCh <- struct{}{}` 确实在 `errCh <-
err` 之后（`listener_manager.go:216-220`），所以 `allDone` 关闭时所有错误已入队，`drainErrors`
不会漏。这点你处理对了。
--
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]
