fix bug: incorrectly defined storeToken using uint in warmup flow control #218
Conversation
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
==========================================
- Coverage 42.58% 42.54% -0.04%
==========================================
Files 82 82
Lines 4405 4409 +4
==========================================
Hits 1876 1876
- Misses 2289 2293 +4
Partials 240 240
Continue to review full report at Codecov.
|
| @@ -41,64 +41,67 @@ func NewWarmUpTrafficShapingCalculator(rule *FlowRule) *WarmUpTrafficShapingCalc | |||
| maxToken: maxToken, | |||
| slope: slope, | |||
| threshold: rule.Count, | |||
| storedTokens: new(uint64), | |||
| lastFilledTime: new(uint64), | |||
| storedTokens: 0, | |||
There was a problem hiding this comment.
The default value is 0 and no assignment is required。
There was a problem hiding this comment.
Yes, just an explicit declaration
| aboveToken := restToken - d.warningToken | ||
| warningQps := math.Nextafter(1.0/(float64(aboveToken)*d.slope+1.0/d.threshold), math.MaxFloat64) | ||
| restToken := atomic.LoadInt64(&c.storedTokens) | ||
| if restToken < 0 { |
There was a problem hiding this comment.
There is no situation where the restToken is less than 0.
There was a problem hiding this comment.
It might have;
if atomic.CompareAndSwapInt64(&c.storedTokens, oldValue, newValue) {
if currentValue := atomic.AddUint64(d.storedTokens, uint64(0-passQps)); currentValue < 0 { if currentValue := atomic.AddInt64(&c.storedTokens, int64(-passQps)); currentValue < 0 {
In concurrency, after currentValue := atomic.AddInt64(&c.storedTokens, int64(-passQps)); might cause storedTokens is less than 0;
|
Thanks! |
Describe what this PR does / why we need it
sentinel-golang/core/flow/tc_warm_up.go
Line 21 in 36a09c5
Define storedTokens as uint64 might cause bug.
sentinel-golang/core/flow/tc_warm_up.go
Line 78 in 36a09c5
Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews