Repository navigation
fix: reconnect attempts after successful pool fill - #140
AlexStocks merged 4 commits into
Conversation
222twotwotwo
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes reconnect attempt accounting so that successful connection pool fills do not consume the reconnect failure budget, and adds regression tests covering bounded reconnect exit and successful pool fills when ConnectionNumber > ReconnectAttempts.
Changes:
- Refactors
connect()to return whether it successfully added a session, and updatesreConnect()to count only consecutive failures (with capped backoff). - Simplifies dial paths (TCP/UDP/WS/WSS) to single-attempt behavior and propagates TLS-config builder failures cleanly.
- Adds regression tests for max-attempt reconnect exit and for successful multi-session pool fills with a small reconnect budget.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| client.go | Updates dial/connect/reconnect logic to track reconnect attempts by consecutive failures and to reset on successful session adds. |
| client_test.go | Adds regression tests validating reconnect attempt bounds and successful connection pool fills. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| c.Unlock() | ||
| ss.SetAttribute(sessionClientKey, c) | ||
| ss.SetAttribute(ignoreReconnectKey, false) | ||
| break | ||
| return true |
| } | ||
| clt.RunEventLoop(cb) | ||
|
|
||
| assert.Equal(t, connPoolSize, msgHandler.SessionNumber()) |
There was a problem hiding this comment.
[P1] 让测试真正覆盖成功后重置连续失败计数
这个用例只有连续成功建满连接池。删除 client.go 中的 reconnectAttempts = 0 后，我在 exact Head mutant 上执行 go test -count=50 . -run "TestTCPClientReconnect(StopsAfterMaxAttempts|AttemptsIgnoreSuccessfulPoolFills)$" 仍全部通过，因此它没有覆盖本 PR 的关键行为。错误实现会把成功前的失败次数带到后续重连，导致‘失败 -> 成功 -> 再次失败’时提前耗尽上限并留下未填满的连接池。请把建连结果做成可控序列，至少覆盖失败、成功、再次失败和再次成功；断言每次成功都会重置连续失败计数，并确认删除重置语句后测试稳定失败。
ee1973f to
cd64a48
Compare
| // gorilla/websocket/conn.go:(Conn)Close also invoke net.Conn.Close() | ||
| _ = ss.Conn().Close() | ||
| } | ||
| return false |
There was a problem hiding this comment.
[P1] 这个 return false 只有 dial 失败并跳出 connect() 内部循环后才会执行。只要 dial 成功而 c.newSession(ss) 持续返回错误，当前实现就会关闭连接后立即在 connect() 内再次 dial，永远不会回到 reConnect() 增加 reconnectAttempts 或执行 backoff。当前 Head 的定向测试配置最大尝试次数为 2，但 RunEventLoop 超过 500ms 仍未返回。请让 connect() 一次只完成一次 dial/初始化尝试；newSession 失败时关闭 session 并返回 false，由外层统一计数和退避，同时补“dial 成功、newSession 连续失败”的回归测试。
There was a problem hiding this comment.
收到，已将connect()从内部循环改为单次尝试，并补充测试
Address CodeRabbit review on PR apache#140: - TestReconnectTriggerDuringBackoffIsCoalesced: keep the reconnect interval far above the 50ms assertion window so CI scheduling delay cannot start attempt 2 inside the window; Close() cancels the backoff so the test stays fast. - TestTCPClientOnOpenCloseKeepsPoolAtConfiguredSize: inspect ssMap before calling sessionNum(), which prunes closed sessions and would otherwise hide them from the closed-session count.
What this PR does:
Fixes reconnect attempt accounting so successful pool fills no longer consume the reconnect failure budget.
connect()now reports whether it actually added a session, andreConnect()only incrementsreconnectAttemptswhen no session was added.Also adds regression coverage for:
ConnectionNumber > ReconnectAttemptsWhich issue(s) this PR fixes:
Fixes #132
Special notes for your reviewer:
This keeps the upstream-style single-attempt dial behavior, while making retry accounting track consecutive failures rather than successful pool-fill progress.
Validated with:
Does this PR introduce a user-facing change?: