[fix] Panic while accessing SleepyTicker methods Stop()/SetSleep() #1779
Conversation
The time.Ticker object was stored as a value type, but it is expected to be a pointer according to its implementation: ``` func (t *Ticker) Stop() func (t *Ticker) Reset(d Duration) ``` This was leading to an application crash. STR 1: Run `portmaster-core` without privileged rights. It will not be able to start the kernel driver (Windows). During unloading of already initialized modules, the process crashes because of stopping SleepyTicker instances in workers of the "network" module. STR 2: Run tests from `service\mgr\sleepyticker_test.go`
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ast-grep (0.31.1)service/mgr/sleepyticker.goAn unexpected error occurred while running ast-grep. 📝 WalkthroughWalkthroughThe pull request introduces changes to the Changes
Sequence DiagramsequenceDiagram
participant ST as SleepyTicker
participant Ticker as time.Ticker
ST->>Ticker: NewTicker(normalDuration)
ST->>ST: Wait for tick
alt Normal Mode
Ticker-->>ST: Tick received
else Sleep Mode
ST->>Ticker: SetSleep(sleepDuration)
Ticker-->>ST: No tick
end
ST->>Ticker: Stop()
ST->>ST: No further ticks
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
service/mgr/sleepyticker_test.go (3)
8-14: Add t.Parallel() for concurrent test executionThe test effectively verifies that
Stop()no longer panics. Consider addingt.Parallel()at the start of the test to enable concurrent execution with other tests.func TestSleepyTickerStop(t *testing.T) { + t.Parallel() normalDuration := 100 * time.Millisecond🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: Function TestSleepyTickerStop missing the call to method parallel
(paralleltest)
16-57: Consider using sub-tests and parallel executionThe test comprehensively covers the key scenarios. Consider these improvements:
- Add
t.Parallel()for concurrent execution- Use
t.Run()to organize scenarios into sub-tests for better isolation and parallel executionfunc TestSleepyTicker(t *testing.T) { + t.Parallel() normalDuration := 100 * time.Millisecond sleepDuration := 200 * time.Millisecond - st := NewSleepyTicker(normalDuration, sleepDuration) - - // Test normal mode + t.Run("normal mode", func(t *testing.T) { + t.Parallel() + st := NewSleepyTicker(normalDuration, sleepDuration) + defer st.Stop() select {Continue this pattern for other test scenarios (sleep mode, zero sleep duration, and stop).
🧰 Tools
🪛 golangci-lint (1.62.2)
16-16: Function TestSleepyTicker missing the call to method parallel
(paralleltest)
1-57: Consider adding more test casesWhile the current tests cover the main functionality, consider adding these scenarios:
- Calling
SetSleep(true)multiple times- Switching between sleep and normal modes rapidly
- Edge cases with very small or very large durations
- Concurrent access to Wait() from multiple goroutines
Would you like me to provide example implementations for these test cases?
🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: Function TestSleepyTickerStop missing the call to method parallel
(paralleltest)
16-16: Function TestSleepyTicker missing the call to method parallel
(paralleltest)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
service/mgr/sleepyticker.go(2 hunks)service/mgr/sleepyticker_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
service/mgr/sleepyticker_test.go
8-8: Function TestSleepyTickerStop missing the call to method parallel
(paralleltest)
16-16: Function TestSleepyTicker missing the call to method parallel
(paralleltest)
🔇 Additional comments (4)
service/mgr/sleepyticker.go (4)
7-7: Correct fix: Changed ticker to pointer type
The change from time.Ticker to *time.Ticker is the right fix. The time.Ticker type's methods like Stop() and Reset() are defined with pointer receivers, so storing it as a value type would create a copy, leading to method calls on the copy instead of the original ticker. This explains the panic in the original code.
19-19: LGTM: Correct initialization of ticker pointer
The initialization is properly updated to match the new pointer type field. time.NewTicker returns *time.Ticker which is now directly assigned without dereferencing.
Line range hint 35-37: Verified: Stop() now works correctly with pointer ticker
The Stop() method now correctly operates on the original ticker instance rather than a copy, preventing the panic that was occurring in the original code.
✅ Verification successful
Based on the results, I can now generate the final response:
Verified: Stop() implementation correctly handles ticker cleanup
The verification confirms that:
SleepyTickerstruct holds a pointer totime.Ticker(ticker *time.Ticker)- The
Stop()method correctly callsStop()on the underlying ticker pointer (st.ticker.Stop()) - Test files demonstrate the method is working as expected without panics
- The implementation is used consistently across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that Stop() is only called on pointer receiver
ast-grep --pattern 'func ($r *time.Ticker) Stop()'
Length of output: 52
Script:
#!/bin/bash
# Let's check the implementation of SleepyTicker's Stop method and its usage
ast-grep --pattern 'type SleepyTicker struct {
$$$
}'
# Also check for any Stop method implementations
ast-grep --pattern 'func ($r *SleepyTicker) Stop() {
$$$
}'
# And check for any usages of Stop
rg "\.Stop\(\)" -A 2
Length of output: 8189
Line range hint 40-56: Verified: SetSleep() now works correctly with pointer ticker
The SetSleep() method now correctly calls Reset() on the original ticker instance. The channel management logic for sleep mode is well-implemented, properly handling both the sleep and wake scenarios.
✅ Verification successful
Verified: SetSleep() correctly uses pointer ticker for Reset() operations
The verification confirms that:
SleepyTicker.tickeris properly declared as*time.Ticker- The ticker is correctly initialized as a pointer in
NewSleepyTicker()usingtime.NewTicker() Reset()calls inSetSleep()are made on the pointer ticker, which is the correct way to use thetime.TickerAPI
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that Reset() is only called on pointer receiver
ast-grep --pattern 'func ($r *time.Ticker) Reset($d time.Duration)'
Length of output: 69
Script:
#!/bin/bash
# Let's verify the implementation of SleepyTicker and its usage of Reset
ast-grep --pattern 'type SleepyTicker struct {
$$$
}'
# Also check the initialization to ensure ticker is a pointer
ast-grep --pattern 'func NewSleepyTicker($$$) *SleepyTicker {
$$$
}'
# And check all Reset calls within the codebase
rg "Reset\(" -A 2
Length of output: 3818
The time.Ticker object was stored as a value type, but it is expected to be a pointer according to its implementation:
This was leading to an application crash.
STR 1:
Run
portmaster-corewithout privileged rights. It will not be able to start the kernel driver (Windows). During unloading of already initialized modules, the process crashes because of stopping SleepyTicker instances in workers of the "network" module.STR 2:
Run tests from
service\mgr\sleepyticker_test.goSummary by CodeRabbit
New Features
SleepyTickerfunctionality, validating its behavior in normal and sleep modes.Bug Fixes
Refactor
tickerfield from a value type to a pointer type for improved memory management and performance.