fix(channel): handle dynamic frequency updates - #2002
Conversation
- replace infinite sleep loop with time.Ticker to avoid goroutine leaks - add immediate initial test execution before ticker starts - implement frequency change detection and ticker recreation - ensure proper ticker cleanup when loop exits or feature disabled
WalkthroughMoves frequency retrieval for AutoTestChannelMinutes into the inner loop of the channel-test scheduler, logs the interval each inner iteration, adds a log after each Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant G as Goroutine (scheduler)
participant Conf as Config (enabled / minutes)
participant T as Ticker
participant R as testAllChannels
Note over G,Conf: Outer loop reads enabled flag once, enters inner loop when enabled
G->>Conf: read AutoTestChannelEnabled
alt enabled
G->>R: run testAllChannels
R-->>G: done
G->>G: log "automatically channel test finished"
G->>T: create ticker
loop each tick
T-->>G: tick
G->>Conf: re-read AutoTestChannelMinutes
G->>G: log current interval
G->>Conf: re-check AutoTestChannelEnabled
alt disabled
G-->>T: stop ticker
G-->>G: break inner loop
else enabled
G->>R: run testAllChannels
R-->>G: done
G->>G: log "automatically channel test finished"
alt frequency changed
G-->>T: stop ticker
G->>T: recreate ticker(new freq)
end
end
end
else disabled
G-->>G: exit (no-op)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controller/channel-test.go (1)
618-663: No graceful shutdown mechanism.The goroutine spawned at line 620 runs indefinitely with no way to cancel it during application shutdown except by setting
AutoTestChannelEnabledto false. This can delay shutdown or leave the goroutine running until the next sleep/tick completes.Accept a
context.Contextparameter and monitor it for cancellation:-func AutomaticallyTestChannels() { +func AutomaticallyTestChannels(ctx context.Context) { autoTestChannelsOnce.Do(func() { go func() { for { + select { + case <-ctx.Done(): + common.SysLog("automatic channel testing stopped via context cancellation") + return + default: + } + if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { time.Sleep(1 * time.Minute) continue } // ... rest of the logic func() { defer ticker.Stop() // ... for { select { + case <-ctx.Done(): + common.SysLog("automatic channel testing stopped via context cancellation") + return case <-ticker.C: // ... } } }() } }() }) }Then call it from your initialization code with a cancellable context:
ctx, cancel := context.WithCancel(context.Background()) defer cancel() // cancel on shutdown AutomaticallyTestChannels(ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/channel-test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controller/channel-test.go (2)
setting/operation_setting/monitor_setting.go (1)
GetMonitorSetting(25-34)common/sys_log.go (1)
SysLog(11-14)
🔇 Additional comments (2)
controller/channel-test.go (2)
631-659: Ticker lifecycle management looks correct.The anonymous function pattern with
defer ticker.Stop()(line 633) ensures the ticker is properly cleaned up when the function returns due to configuration changes (lines 646, 650) or when the loop exits. This prevents goroutine and ticker leaks.
635-638: Good: immediate initial test execution.Running the test immediately before starting the ticker loop ensures tests begin without waiting for the first interval to elapse, which improves user experience when the feature is enabled.
| for { | ||
| time.Sleep(time.Duration(frequency) * time.Minute) | ||
| common.SysLog("automatically testing all channels") | ||
| _ = testAllChannels(false) | ||
| common.SysLog("automatically channel test finished") | ||
| if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { | ||
| break | ||
| time.Sleep(10 * time.Minute) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Slow reaction time when re-enabling the feature.
When AutoTestChannelEnabled is false, the code sleeps for a hardcoded 10 minutes before rechecking. If an operator re-enables the feature, they must wait up to 10 minutes before tests resume.
Consider reducing the sleep interval (e.g., 1 minute) or using a configuration change notification mechanism for faster response:
for {
if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled {
- time.Sleep(10 * time.Minute)
+ time.Sleep(1 * time.Minute)
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for { | |
| time.Sleep(time.Duration(frequency) * time.Minute) | |
| common.SysLog("automatically testing all channels") | |
| _ = testAllChannels(false) | |
| common.SysLog("automatically channel test finished") | |
| if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { | |
| break | |
| time.Sleep(10 * time.Minute) | |
| continue | |
| } | |
| for { | |
| if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { | |
| time.Sleep(1 * time.Minute) | |
| continue | |
| } |
🤖 Prompt for AI Agents
In controller/channel-test.go around lines 621-625, the loop sleeps for a
hardcoded 10 minutes when AutoTestChannelEnabled is false causing slow reaction
to re-enabling; change this to either poll more frequently (e.g., sleep 1
minute) or, preferably, replace the sleep with a configurable interval from
operation_setting (e.g., MonitorSetting.AutoTestPollInterval) or a notification
mechanism: implement a ticker/select that checks the flag at the shorter
interval (or listens on a config change channel) so the loop can exit or resume
promptly when AutoTestChannelEnabled flips.
| case <-ticker.C: | ||
| // 检查是否需要退出或重新配置 | ||
| if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { | ||
| return | ||
| } | ||
| newFrequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes | ||
| if newFrequency != frequency { | ||
| return | ||
| } |
There was a problem hiding this comment.
Frequency changes are not detected immediately.
The frequency change detection (lines 648-650) only runs after the ticker fires, meaning if the frequency changes from 60 minutes to 5 minutes, the system continues using the old 60-minute interval until the next tick occurs. This delays the effect of dynamic frequency updates by up to one full interval of the old frequency.
For truly dynamic updates, consider one of these approaches:
- Poll configuration more frequently (e.g., every minute) in a separate select case
- Use a configuration change notification channel to trigger immediate recreation
- Document this behavior if delayed reaction is acceptable
Example using periodic polling:
+configCheckTicker := time.NewTicker(1 * time.Minute)
+defer configCheckTicker.Stop()
+
for {
select {
case <-ticker.C:
- // 检查是否需要退出或重新配置
- if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled {
- return
- }
- newFrequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes
- if newFrequency != frequency {
- return
- }
-
// 执行定时任务
common.SysLog("automatically testing all channels")
_ = testAllChannels(false)
common.SysLog("automatically channel test finished")
+
+ case <-configCheckTicker.C:
+ // 检查是否需要退出或重新配置
+ if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled {
+ return
+ }
+ newFrequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes
+ if newFrequency != frequency {
+ common.SysLog(fmt.Sprintf("frequency changed from %d to %d minutes, recreating ticker", frequency, newFrequency))
+ return
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case <-ticker.C: | |
| // 检查是否需要退出或重新配置 | |
| if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { | |
| return | |
| } | |
| newFrequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes | |
| if newFrequency != frequency { | |
| return | |
| } | |
| // 在进入循环前增加一个更频繁的配置检查 | |
| configCheckTicker := time.NewTicker(1 * time.Minute) | |
| defer configCheckTicker.Stop() | |
| for { | |
| select { | |
| case <-ticker.C: | |
| // 执行定时任务 | |
| common.SysLog("automatically testing all channels") | |
| _ = testAllChannels(false) | |
| common.SysLog("automatically channel test finished") | |
| case <-configCheckTicker.C: | |
| // 检查是否需要退出或重新配置 | |
| if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { | |
| return | |
| } | |
| newFrequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes | |
| if newFrequency != frequency { | |
| common.SysLog(fmt.Sprintf( | |
| "frequency changed from %d to %d minutes, recreating ticker", | |
| frequency, newFrequency, | |
| )) | |
| return | |
| } | |
| } | |
| } |
|
感觉你搞复杂了，直接把那个获取时间的和日志挪进去内层循环不就可以嘛 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/channel-test.go (1)
626-634: Frequency changes take effect only after the current interval completes.While the code retrieves
frequencyon each iteration (line 626), it doesn't check whether the frequency has changed during the sleep. If the frequency updates from 60 minutes to 5 minutes, the system continues waiting the full 60 minutes before the next test, then adopts 5 minutes for subsequent iterations.Impact: Operators must wait up to one full interval (at the old frequency) before dynamic frequency updates take effect, contradicting the PR objective: "Implement detection of frequency changes and recreation of the ticker when frequency updates occur."
Based on learnings from the past review comment, consider polling the configuration more frequently to detect changes:
for { frequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes + common.SysLog(fmt.Sprintf("automatically test channels with interval %d minutes", frequency)) + common.SysLog("automatically testing all channels") + _ = testAllChannels(false) + common.SysLog("automatically channel test finished") + + // Poll for config changes every minute during the sleep period + sleepRemaining := time.Duration(frequency) * time.Minute + pollInterval := 1 * time.Minute + for sleepRemaining > 0 { + if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { + break + } + newFrequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes + if newFrequency != frequency { + common.SysLog(fmt.Sprintf("frequency changed from %d to %d minutes, restarting", frequency, newFrequency)) + break + } + sleepDuration := pollInterval + if sleepRemaining < pollInterval { + sleepDuration = sleepRemaining + } + time.Sleep(sleepDuration) + sleepRemaining -= sleepDuration + } - time.Sleep(time.Duration(frequency) * time.Minute) - common.SysLog(fmt.Sprintf("automatically test channels with interval %d minutes", frequency)) - common.SysLog("automatically testing all channels") - _ = testAllChannels(false) - common.SysLog("automatically channel test finished") if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { break } }This approach also enables faster detection when the feature is disabled mid-interval.
♻️ Duplicate comments (1)
controller/channel-test.go (1)
622-622: Slow reaction time when re-enabling the feature remains unaddressed.The 10-minute sleep when
AutoTestChannelEnabledis false (line 622) was flagged in a previous review as causing delays of up to 10 minutes before tests resume after re-enabling the feature. This concern has not been addressed in the current changes.Per the past review feedback, consider reducing the sleep interval to 1 minute for faster response:
if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { - time.Sleep(10 * time.Minute) + time.Sleep(1 * time.Minute) continue }Based on learnings from past review.
🧹 Nitpick comments (1)
controller/channel-test.go (1)
628-628: Log timing could be clearer.The log "automatically test channels with interval X minutes" appears after the sleep but before the test execution, which creates a slightly confusing timeline. If the sleep is moved to the end of the loop (as suggested in the critical issue above), this log would naturally appear at the start of each iteration, providing clearer context: "Starting new cycle with X-minute interval."
This will be automatically resolved if the sleep is moved to the end of the loop as recommended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/channel-test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controller/channel-test.go (2)
setting/operation_setting/monitor_setting.go (1)
GetMonitorSetting(25-34)common/sys_log.go (1)
SysLog(11-14)
| frequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes | ||
| time.Sleep(time.Duration(frequency) * time.Minute) | ||
| common.SysLog(fmt.Sprintf("automatically test channels with interval %d minutes", frequency)) |
There was a problem hiding this comment.
Critical: Sleep-before-test violates the "immediate initial test" objective.
The current implementation retrieves the frequency and immediately sleeps (lines 626-627) before performing any test. This means:
- No immediate initial test: When the inner loop starts, it waits the full interval before the first test runs, contradicting the PR description which explicitly states "Add immediate initial test execution before the ticker starts."
- Delayed first execution: If the frequency is 60 minutes, operators must wait a full hour before the first test, even though the feature was just enabled.
The correct flow should be: test → sleep → test, not sleep → test. Move the sleep to the end of the loop:
for {
frequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes
- time.Sleep(time.Duration(frequency) * time.Minute)
common.SysLog(fmt.Sprintf("automatically test channels with interval %d minutes", frequency))
common.SysLog("automatically testing all channels")
_ = testAllChannels(false)
common.SysLog("automatically channel test finished")
if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled {
break
}
+ time.Sleep(time.Duration(frequency) * time.Minute)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| frequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes | |
| time.Sleep(time.Duration(frequency) * time.Minute) | |
| common.SysLog(fmt.Sprintf("automatically test channels with interval %d minutes", frequency)) | |
| for { | |
| frequency := operation_setting.GetMonitorSetting().AutoTestChannelMinutes | |
| common.SysLog(fmt.Sprintf("automatically test channels with interval %d minutes", frequency)) | |
| common.SysLog("automatically testing all channels") | |
| _ = testAllChannels(false) | |
| common.SysLog("automatically channel test finished") | |
| if !operation_setting.GetMonitorSetting().AutoTestChannelEnabled { | |
| break | |
| } | |
| time.Sleep(time.Duration(frequency) * time.Minute) | |
| } |
🤖 Prompt for AI Agents
In controller/channel-test.go around lines 626 to 628, the loop currently reads
the frequency and immediately calls time.Sleep before performing the test, which
prevents an immediate initial test; change the loop order so the test logic runs
first, then call time.Sleep(time.Duration(frequency) * time.Minute) at the end
of the loop (keeping frequency retrieval as-is or validating it once outside the
loop), and ensure you handle zero/negative frequency (skip sleeping or use a
sensible default) so the execution sequence becomes: test → sleep → test.
…ncy-updates fix(channel): handle dynamic frequency updates
2 participants
Summary by CodeRabbit