fix: serverIPListComponent not close when invoke Client close#350
fix: serverIPListComponent not close when invoke Client close#350Epiphany523 wants to merge 5 commits intoapolloconfig:developfrom
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 You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
|
感谢您提出Pull Request，我会尽快Review。我会在1-2日内进行查看或者回复，如果遇到节假日可能会处理较慢，敬请谅解。 |
There was a problem hiding this comment.
感谢你的贡献！这个 PR 修复了一个重要的资源泄漏问题 👍
✅ 优点
- 问题定位准确: 确实修复了 serverIPListComponent 和 ConfigComponent 的资源泄漏问题
- 设计改进: 统一使用
components数组管理所有组件，代码更清晰 - 符合最佳实践:
- stopCh 在构造函数中初始化，避免并发问题
- 使用
chan struct{}替代chan interface{}，更符合 Go 惯例
- 日志完善: 添加了组件启动和停止的 debug 日志
⚠️ 需要改进的地方
1. ConfigComponent 缺少 Stop() 方法
component/notify/componet_notify.go 中 ConfigComponent 实现了 AbsComponent 接口，但缺少 Stop() 方法的实现。建议添加：
func (c *ConfigComponent) Stop() {
if c.stopCh != nil {
close(c.stopCh)
}
}2. 并发安全问题
Stop() 方法中关闭 channel 可能存在重复关闭的风险。建议使用 sync.Once 保护：
type SyncServerIPListComponent struct {
appConfig func() config.AppConfig
stopCh chan struct{}
stopOnce sync.Once
}
func (s *SyncServerIPListComponent) Stop() {
s.stopOnce.Do(func() {
if s.stopCh != nil {
close(s.stopCh)
}
})
}ConfigComponent 同样需要此保护。
3. 废弃方法处理
SetAppConfig 和 SetCache 方法已经不再使用，建议：
- 标记为
Deprecated并在注释中说明使用NewConfigComponent替代 - 或者直接删除（如果确认没有外部调用）
// SetAppConfig Deprecated: use NewConfigComponent instead
func (c *ConfigComponent) SetAppConfig(appConfigFunc func() config.AppConfig) {
c.appConfigFunc = appConfigFunc
}4. 测试覆盖
建议添加测试用例验证：
- Client.Close() 能正确停止所有组件
- 重复调用 Close() 不会 panic
- 组件停止后 goroutine 正确退出
5. 代码注释格式
部分注释格式不一致，建议统一：
// InitSyncServerIPList 初始化同步服务器信息列表🔍 其他建议
- 错误处理: component.StartRefreshConfig 如果 panic，可能导致程序崩溃，建议添加 recover
- 优雅关闭: 可以考虑添加超时机制，避免 Stop() 无限等待
总结
这是一个很好的 bug 修复，解决了实际的资源泄漏问题。主要需要：
- 必须添加: ConfigComponent.Stop() 方法
- 强烈建议: 添加并发安全保护（sync.Once）
- 建议: 添加测试用例
修复第 1、2 点后可以合并。
本次的修改变更
func (c *ConfigComponent) Stop() {
c.stopOnce.Do(func() {
if c.stopCh != nil {
close(c.stopCh)
}
})
}
func (s *SyncServerIPListComponent) Stop() {
s.stopOnce.Do(func() {
if s.stopCh != nil {
close(s.stopCh)
}
})
}
远景合并后，是否可以评估出一个修复该问题的版本的时间 |
|
感谢更新！对照上次建议，以下已完成：
新增需要关注的问题：
关于 SetAppConfig/SetCache： 关于发布时间： 若修复第 1 点和第 2 点，我倾向于可以合并。 |
本次修改变更兼容性回归:
panic 日志格式： 修改成 |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical bug where the SyncServerIPListComponent was not properly stopped when Client.Close() was invoked. The PR refactors the component lifecycle management to ensure all components can be properly shut down.
Changes:
- Added a
Stop()method to theAbsComponentinterface and implemented proper shutdown logic for bothConfigComponentandSyncServerIPListComponent - Refactored client to track all components in a slice and stop them all when
Close()is called - Introduced constructor functions (
NewConfigComponent,NewSyncServerIPListComponent) to ensure components are properly initialized with stop channels
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| client.go | Changed from storing single configComponent to components slice; added appendComponent() method; updated Close() to stop all components |
| component/common.go | Added Stop() method to AbsComponent interface; added panic recovery in StartRefreshConfig |
| component/notify/componet_notify.go | Added stopCh, stopOnce fields; implemented Stop() method; added NewConfigComponent constructor; deprecated SetAppConfig and SetCache |
| component/serverlist/sync.go | Added stopCh, stopOnce fields; implemented Stop() method; added NewSyncServerIPListComponent constructor; deprecated InitSyncServerIPList |
| component/notify/componet_notify_test.go | Added test for Stop() method covering nil channel and duplicate stop scenarios |
| component/serverlist/sync_test.go | Added test for Stop() method covering nil channel and duplicate stop scenarios |
| client_test.go | Added test for Close() method verifying all components are stopped |
| component/common_test.go | Updated testComponent to implement new Stop() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *SyncServerIPListComponent) Start() { | ||
| SyncServerIPList(s.appConfig) | ||
| log.Debug("syncServerIpList started") | ||
| log.Debug("syncServerIpListComponent started") | ||
|
|
||
| t2 := time.NewTimer(refreshIPListInterval) | ||
| for { | ||
| select { | ||
| case <-s.stopCh: | ||
| log.Debug("syncServerIpListComponent stopped") | ||
| return | ||
| case <-t2.C: | ||
| SyncServerIPList(s.appConfig) | ||
| t2.Reset(refreshIPListInterval) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The Timer created in the Start method is not properly cleaned up when the component stops. When the stopCh receives a signal and the function returns, the timer t2 will continue to exist and consume resources. You should call t2.Stop() before returning to release the timer resources and prevent goroutine leaks.
| func (c *internalClient) Close() { | ||
| c.configComponent.Stop() | ||
| for _, comp := range c.components { | ||
| comp.Stop() | ||
| } | ||
| } |
There was a problem hiding this comment.
The Close method has a potential race condition. Multiple goroutines could call Close concurrently, and even though each component's Stop method is protected by sync.Once, the iteration over c.components is not thread-safe. If one goroutine is iterating through the components slice while another goroutine calls appendComponent, this could lead to undefined behavior. Consider adding a mutex to protect access to c.components or ensuring Close is only called once using a sync.Once pattern at the client level.
| @@ -269,6 +269,10 @@ func (c *internalClient) getConfigValue(key string) interface{} { | |||
| return value | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The appendComponent method has a potential race condition. It modifies the c.components slice without synchronization, which could lead to data races if called concurrently with Close or with other appendComponent calls. While in the current implementation all appendComponent calls happen during initialization before components start, this creates a fragile design. Consider adding mutex protection or documenting that this method must only be called during initialization.
| // appendComponent adds a component to the internal list. | |
| // It is not safe for concurrent use and must only be called during client | |
| // initialization, before any components are started or Close may be invoked. |
component/common.go
Outdated
| func StartRefreshConfig(component AbsComponent) { | ||
| defer func() { | ||
| if err := recover(); err != nil { | ||
| log.Errorf("StartRefreshConfig component start failed,recover panic: %v", err) |
There was a problem hiding this comment.
Missing space after comma in the log message. The message should be "StartRefreshConfig component start failed, recover panic: %v" instead of "StartRefreshConfig component start failed,recover panic: %v" for consistency with other error messages in the codebase. Looking at other log.Errorf calls in the codebase (e.g., component/common_test.go:152, component/serverlist/sync.go:129), there is a space after commas in error messages.
| log.Errorf("StartRefreshConfig component start failed,recover panic: %v", err) | |
| log.Errorf("StartRefreshConfig component start failed, recover panic: %v", err) |
client_test.go
Outdated
| Assert(t, tc.status, Equal(0)) | ||
| Assert(t, tc2.status, Equal(0)) | ||
| c.Close() | ||
| c.Close() // replicate Close |
There was a problem hiding this comment.
The comment uses "replicate" when it should be "duplicate" or "repeated". The word "replicate" means to reproduce or make a copy, while "duplicate" means to do something again. In this context, the test is calling Close twice to verify idempotency, so "duplicate Close" or "repeated Close" would be more appropriate.
| c.Close() // replicate Close | |
| c.Close() // duplicate Close |
| @@ -66,14 +75,17 @@ loop: | |||
| } | |||
| t2.Reset(longPollInterval) | |||
| case <-c.stopCh: | |||
| break loop | |||
| log.Debug("ConfigComponent stopped") | |||
| return | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
The Timer created in the Start method is not properly cleaned up when the component stops. When the stopCh receives a signal and the function returns, the timer t2 will continue to exist and consume resources. You should call t2.Stop() before returning to release the timer resources and prevent goroutine leaks.
|
感谢补充修改。这里有两个可能影响兼容性的点，请结合 agollo 的实际使用场景判断一下：
|
对于兼容性,本次修改的个人理解：（结合 agollo 的实际使用场景）用户使用场景： 用户使用 对于AbsComponent定位
旧用法 &ConfigComponent{} + SetAppConfig/SetCache + Start/Stop 旧用法虽然会暴露给用户，但是用户没有使用场景 本次变更将 |
|
感谢这轮更新， 这里我建议本 PR 内再补一个点（偏 must-fix）：
另外，在前述讨论里我看到两个兼容性问题：
这两点虽然是问题，但是否需要在本 PR 内处理，还是要结合 agollo 的实际使用场景和兼容策略来判断。@zouyx 能否也帮忙确认下这两点是否 OK？ |
2 participants
problem1 desc
serverIPListComponent not close when invoke Client close
problem2 desc
ConfigComponent.stopCh should init when ConfigComponent struct create, not ConfigComponent.Start
test