Repository navigation
Fix client shutdown deadlock - #142
Conversation
| close(done) | ||
| return done | ||
| default: | ||
| c.wg.Add(1) |
There was a problem hiding this comment.
[P1] 这里把整个重连 goroutine 计入 wg 后,NewSessionCallback 内调用公开的 Client.Close() 会等待当前 goroutine 自己,形成确定性死锁。固定 Base/Head 的公开入口探针中,Base 在回调调用 Close() 并返回错误后正常退出;当前 Head 在 500ms 内始终无法返回。回调文档没有禁止关闭所属 endpoint,这类连接校验失败后主动停机的用法不能被锁死。请避免让 Close() 等待正在执行该回调的 worker(例如把停止信号与外部 join 分离,或调整重连任务的生命周期登记),并补一个回调内关闭客户端的回归测试。
There was a problem hiding this comment.
收到, NewSessionCallback内调用 Client.Close() 时不会再等待当前正在执行 callback 的 goroutine 自己。
There was a problem hiding this comment.
Pull request overview
This PR addresses a client lifecycle deadlock during shutdown by decoupling session teardown from reconnect logic and ensuring reconnect work is coordinated and cancellable via client shutdown.
Changes:
- Routes session-triggered reconnect through a new
client.runReconnect()helper instead of callingreConnect()inline from session teardown. - Updates
client.stop()to snapshot and close sessions outside the client lock to avoid lock re-entrancy during shutdown. - Adds regression tests covering shutdown/reconnect interleavings and reconnect backoff cancellation behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| session.go | Switches session teardown from synchronous reConnect() to asynchronous, tracked runReconnect() to avoid shutdown lock re-entry. |
| client.go | Introduces runReconnect() (tracked via WaitGroup), waits for initial reconnect in RunEventLoop, and closes sessions outside the client lock during stop(). |
| client_test.go | Adds regression tests for reconnect/backoff cancellation and shutdown/reconnect interleavings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…eadlock Fix client shutdown deadlock
What this PR does:
Prevents client shutdown from deadlocking or hanging when session teardown can trigger reconnect logic.
This PR:
ssMapduring shutdownrunReconnect()to start reconnect loops only while the client is open and track them with the clientWaitGrouprunReconnect()instead of synchronously callingreConnect()Client.Close()Which issue(s) this PR fixes:
Fixes #132
Special notes for your reviewer:
The branch has been merged with the latest
origin/masterAdditional tests cover:
Client.Close()is runningClient.Close()cancelling reconnect backoffrunReconnect()becoming a no-op after client shutdownClient.Close()waiting for an in-flight session-triggered reconnectDoes this PR introduce a user-facing change?: