Skip to content

fix: connection 竞态条件修复,改用 select 安全发送#35

Open
NeverENG wants to merge 1 commit into
mainfrom
fix/connection-race
Open

fix: connection 竞态条件修复,改用 select 安全发送#35
NeverENG wants to merge 1 commit into
mainfrom
fix/connection-race

Conversation

@NeverENG
Copy link
Copy Markdown
Owner

@NeverENG NeverENG commented May 20, 2026

Summary

  • Stop() 中移除 close(msgChan)close(msgBuffChan),避免向已关闭 channel 发送导致 panic
  • SendMsg / SendBuffMsg 增加 isClose 前置守卫
  • 改用 select + ctx.Done() 替代无条件 channel 发送,防止 goroutine 阻塞泄漏

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling when sending messages on closed connections with explicit error returns
    • Enhanced connection robustness to prevent operations after closure
    • Refined non-blocking message send behavior for better reliability

Review Change Stack

- Stop() 中移除 close(msgChan) 和 close(msgBuffChan),避免向已关闭 channel 发送导致 panic
- SendMsg/SendBuffMsg 增加 isClose 守卫
- 改用 select + ctx.Done() 替代直接 channel 发送,防止阻塞泄漏

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bc8d5866-5369-4bfa-86a9-e8fb7ce302d3

📥 Commits

Reviewing files that changed from the base of the PR and between 4aada05 and 5062a6e.

📒 Files selected for processing (1)
  • network/banNet/connection.go

📝 Walkthrough

Walkthrough

The connection package refactors message delivery to use buffered channels, implements non-blocking sends with error returns, and simplifies lifecycle cleanup. msgChan becomes buffered (capacity 10), StartWriter's default case now services only the buffered channel, channel closing is removed from Stop, and SendMsg/SendBuffMsg return explicit errors when closed or context-cancelled instead of unconditional sends.

Changes

Connection message handling and lifecycle

Layer / File(s) Summary
Channel buffering and reader initialization
network/banNet/connection.go
NewConnection initializes msgChan with a buffer capacity of 10. StartReader logging is updated to use "commenced/exited gracefully" phrasing.
Writer goroutine and non-blocking send behavior
network/banNet/connection.go
StartWriter logging updated to "Writer commenced/Writer closed" messages. The default branch in the send select statement is modified to service only msgBuffChan instead of both channels.
Connection start/stop lifecycle and channel cleanup
network/banNet/connection.go
Start and Stop logging messages are updated to "Connection established — ID" and "Connection terminated — ID" formats. The explicit channel closing calls in Stop for msgChan and msgBuffChan are removed.
Message send error handling and non-blocking semantics
network/banNet/connection.go
SendMsg and SendBuffMsg now return errors when the connection is closed or the context is done. Both methods use select statements for non-blocking sends instead of prior nil checks and unconditional direct sends that always returned nil.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit hops through channels bright,
Buffering messages left and right,
No more blocking, errors flow,
Graceful closes—watch it go! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: fixing a race condition in connection handling using select for safe sends, which aligns with the PR's core objectives of addressing goroutine leaks and channel safety.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/connection-race
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch fix/connection-race

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant