Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

louyuting
Copy link
Collaborator

@louyuting louyuting commented Nov 21, 2020

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
Copy link

codecov-io commented Nov 21, 2020

Codecov Report

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

Impacted file tree graph

@@            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.

@louyuting louyuting requested a review from sczyh30 November 21, 2020 08:05
@louyuting louyuting added the kind/bug Something isn't working label Nov 21, 2020
@louyuting louyuting added this to the 1.0.0 milestone Nov 21, 2020
@luckyxiaoqiang
Copy link
Collaborator

LGTM.

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

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sczyh30 sczyh30 merged commit 3a70369 into alibaba:master Nov 23, 2020
ljun20160606 pushed a commit to ljun20160606/sentinel-golang that referenced this pull request Nov 26, 2020
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working priority/high Very important, need to be worked with soon but not very urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants