Fix bug when leap array as token bucket(sample bucket is 1) #327

louyuting · 2020-11-21T08:03:22Z

Describe what this PR does / why we need it

Benchmark_SlowRequestRatio_SlotCheck_4-32     	{"timestamp":"2020-11-21 07:11:18.18210","caller":"circuit_breaker.go:345","logLevel":"ERROR","msg":"Failed to get current bucket in slowRequestLeapArray.currentCounter()"}
Provided time timeMillis=1605942677000 is already behind old.BucketStart=1605942678000. now=1605942677999
github.com/alibaba/sentinel-golang/core/stat/base.(*LeapArray).currentBucketOfTime
	/home/ytlou/share/golang/sentinel-golang/core/stat/base/leap_array.go:204
github.com/alibaba/sentinel-golang/core/stat/base.(*LeapArray).CurrentBucket
	/home/ytlou/share/golang/sentinel-golang/core/stat/base/leap_array.go:164
github.com/alibaba/sentinel-golang/core/circuitbreaker.(*slowRequestLeapArray).currentCounter
	/home/ytlou/share/golang/sentinel-golang/core/circuitbreaker/circuit_breaker.go:343
github.com/alibaba/sentinel-golang/core/circuitbreaker.(*slowRtCircuitBreaker).OnRequestComplete
	/home/ytlou/share/golang/sentinel-golang/core/circuitbreaker/circuit_breaker.go:257
github.com/alibaba/sentinel-golang/core/circuitbreaker.(*MetricStatSlot).OnCompleted
	/home/ytlou/share/golang/sentinel-golang/core/circuitbreaker/stat_slot.go:44
github.com/alibaba/sentinel-golang/core/base.(*SlotChain).exit
	/home/ytlou/share/golang/sentinel-golang/core/base/slot_chain.go:278
github.com/alibaba/sentinel-golang/core/base.(*SentinelEntry).Exit.func1
	/home/ytlou/share/golang/sentinel-golang/core/base/entry.go:92
sync.(*Once).doSlow
	/home/ytlou/gosdk/go/src/sync/once.go:66
sync.(*Once).Do
	/home/ytlou/gosdk/go/src/sync/once.go:57
github.com/alibaba/sentinel-golang/core/base.(*SentinelEntry).Exit
	/home/ytlou/share/golang/sentinel-golang/core/base/entry.go:77
command-line-arguments.doCheck
	/home/ytlou/share/golang/sentinel-golang/tests/benchmark/circuitbreaker/circuitbreaker_benchmark_test.go:19
command-line-arguments.Benchmark_SlowRequestRatio_SlotCheck_4.func1
	/home/ytlou/share/golang/sentinel-golang/tests/benchmark/circuitbreaker/circuitbreaker_benchmark_test.go:60
testing.(*B).RunParallel.func1
	/home/ytlou/gosdk/go/src/testing/benchmark.go:768
runtime.goexit
	/home/ytlou/gosdk/go/src/runtime/asm_amd64.s:1374
{"timestamp":"2020-11-21 07:11:18.18210","caller":"entry.go:80","logLevel":"ERROR","msg":"Sentinel internal panic in SentinelEntry.Exit()"}
runtime error: invalid memory address or nil pointer dereference
github.com/alibaba/sentinel-golang/core/base.(*SentinelEntry).Exit.func1.1
	/home/ytlou/share/golang/sentinel-golang/core/base/entry.go:80
runtime.gopanic
	/home/ytlou/gosdk/go/src/runtime/panic.go:969
runtime.panicmem
	/home/ytlou/gosdk/go/src/runtime/panic.go:212
runtime.sigpanic
	/home/ytlou/gosdk/go/src/runtime/signal_unix.go:742
github.com/alibaba/sentinel-golang/core/circuitbreaker.(*slowRtCircuitBreaker).OnRequestComplete
	/home/ytlou/share/golang/sentinel-golang/core/circuitbreaker/circuit_breaker.go:261
github.com/alibaba/sentinel-golang/core/circuitbreaker.(*MetricStatSlot).OnCompleted
	/home/ytlou/share/golang/sentinel-golang/core/circuitbreaker/stat_slot.go:44
github.com/alibaba/sentinel-golang/core/base.(*SlotChain).exit
	/home/ytlou/share/golang/sentinel-golang/core/base/slot_chain.go:278
github.com/alibaba/sentinel-golang/core/base.(*SentinelEntry).Exit.func1
	/home/ytlou/share/golang/sentinel-golang/core/base/entry.go:92
sync.(*Once).doSlow
	/home/ytlou/gosdk/go/src/sync/once.go:66
sync.(*Once).Do
	/home/ytlou/gosdk/go/src/sync/once.go:57
github.com/alibaba/sentinel-golang/core/base.(*SentinelEntry).Exit
	/home/ytlou/share/golang/sentinel-golang/core/base/entry.go:77
command-line-arguments.doCheck
	/home/ytlou/share/golang/sentinel-golang/tests/benchmark/circuitbreaker/circuitbreaker_benchmark_test.go:19
command-line-arguments.Benchmark_SlowRequestRatio_SlotCheck_4.func1
	/home/ytlou/share/golang/sentinel-golang/tests/benchmark/circuitbreaker/circuitbreaker_benchmark_test.go:60
testing.(*B).RunParallel.func1
	/home/ytlou/gosdk/go/src/testing/benchmark.go:768
runtime.goexit
	/home/ytlou/gosdk/go/src/runtime/asm_amd64.s:1374

Does this pull request fix one issue?

Describe how you did it

If the sample count is 1 in leapArray, we guarantee currentBucketOfTime returns the bucket.

Describe how to verify it

Special notes for reviews

codecov-io · 2020-11-21T08:05:01Z

Codecov Report

Merging #327 (2d11791) into master (8d33c7c) will decrease coverage by 0.07%.
The diff coverage is 18.51%.

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
- Coverage   50.91%   50.83%   -0.08%     
==========================================
  Files          68       68              
  Lines        3814     3820       +6     
==========================================
  Hits         1942     1942              
- Misses       1606     1609       +3     
- Partials      266      269       +3

Impacted Files	Coverage Δ
core/stat/base/leap_array.go	`70.40% <0.00%> (-1.47%)`	⬇️
core/circuitbreaker/circuit_breaker.go	`67.22% <20.00%> (-0.93%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d33c7c...2d11791. Read the comment docs.

luckyxiaoqiang · 2020-11-23T03:56:46Z

LGTM.

sczyh30

We may refine the currentCounter method of xxxLeapArray in circuit breakers: return error directly and let callers handle it (instead of just returning nil).

sczyh30

LGTM

…libaba#327) * If the sample count is 1 in leapArray, we guarantee `currentBucketOfTime` returns the bucket. * Also refine method signature of getting counters in circuit breaker modules.

louyuting requested a review from sczyh30 November 21, 2020 08:05

louyuting added the kind/bug Something isn't working label Nov 21, 2020

louyuting added this to the 1.0.0 milestone Nov 21, 2020

sczyh30 added the priority/high Very important, need to be worked with soon but not very urgent label Nov 23, 2020

sczyh30 reviewed Nov 23, 2020

View reviewed changes

louyuting force-pushed the 20201121-fix-leap-array branch 2 times, most recently from ff4d3ea to 2d11791 Compare November 23, 2020 12:09

Fix bug when leap array as token bucket(sample bucket is 1)

2d11791

sczyh30 approved these changes Nov 23, 2020

View reviewed changes

sczyh30 merged commit 3a70369 into alibaba:master Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix bug when leap array as token bucket(sample bucket is 1) #327

Fix bug when leap array as token bucket(sample bucket is 1) #327

Uh oh!

louyuting commented Nov 21, 2020 •

edited

Loading

Uh oh!

codecov-io commented Nov 21, 2020 •

edited

Loading

Uh oh!

luckyxiaoqiang commented Nov 23, 2020

Uh oh!

sczyh30 left a comment

Uh oh!

sczyh30 left a comment

Uh oh!

Uh oh!

Fix bug when leap array as token bucket(sample bucket is 1) #327

Fix bug when leap array as token bucket(sample bucket is 1) #327

Uh oh!

Conversation

louyuting commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

Uh oh!

codecov-io commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

luckyxiaoqiang commented Nov 23, 2020

Uh oh!

sczyh30 left a comment

Choose a reason for hiding this comment

Uh oh!

sczyh30 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

louyuting commented Nov 21, 2020 •

edited

Loading

codecov-io commented Nov 21, 2020 •

edited

Loading