fix: avoid stop race on component stop channel#354
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
|
感谢您提出Pull Request，我会尽快Review。我会在1-2日内进行查看或者回复，如果遇到节假日可能会处理较慢，敬请谅解。 |
There was a problem hiding this comment.
Pull request overview
This PR updates two background “refresh” components to more safely manage their stop channels, aiming to prevent nil stop channels from making goroutines un-stoppable and to reduce data races around stop-channel initialization/closure.
Changes:
- Add a mutex (
stopMu) and anensureStopCh()helper to lazily initializestopCh. - In
Start(), capture the stop channel once (stopCh := ensureStopCh()) and select on that local reference. - In
Stop(), guard close logic withstopOnceandstopMu.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| component/serverlist/sync.go | Adds mutex + ensureStopCh() and uses a local stopCh in the sync loop. |
| component/notify/componet_notify.go | Adds mutex + ensureStopCh() and uses a local stopCh in the long-poll loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| s.stopMu.Lock() | ||
| defer s.stopMu.Unlock() | ||
| if s.stopCh != nil { | ||
| close(s.stopCh) | ||
| } |
| if c.stopCh != nil { | ||
| close(c.stopCh) | ||
| } |
Motivation
Start()andStop()concurrently access a component'sstopCh, which can lead to closing a nil/already-closed channel or inconsistent channel reference.Description
stopMu sync.MutextoConfigComponentandSyncServerIPListComponentto protectstopChlifecycle.ensureStopCh()to safely initializestopChunder lock and return a local snapshot used byStart()to avoid reading a field that may be closed concurrently.Start()to read from the localstopChsnapshot and changeStop()to lock before closing the channel (keptstopOnceto ensure idempotence).component/notify/componet_notify.goandcomponent/serverlist/sync.goand rungofmton modified files.Testing
go test ./component/notify ./component/serverlistand both packages passed.go test ./...which failed during module download due toproxy.golang.orgreturning HTTP 403, which is an external network/proxy issue and not related to these code changes.Codex Task