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 data race of Token result Pool #149

Merged
merged 2 commits into from
May 20, 2020
Merged

Conversation

louyuting
Copy link
Collaborator

Describe what this PR does / why we need it

==================
WARNING: DATA RACE
Read at 0x00c0000a0a00 by goroutine 11:
  github.com/alibaba/sentinel-golang/core/base.(*TokenResult).IsBlocked()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/core/base/result.go:58 +0x351
  github.com/alibaba/sentinel-golang/core/stat.(*StatisticSlot).OnCompleted()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/core/stat/stat_slot.go:32 +0x40
  github.com/alibaba/sentinel-golang/core/base.(*SlotChain).exit()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/core/base/slot_chain.go:181 +0x9f
  github.com/alibaba/sentinel-golang/core/base.(*SentinelEntry).Exit.func1()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/core/base/entry.go:64 +0xa3
  sync.(*Once).doSlow()
      /usr/local/go/src/sync/once.go:66 +0x103
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:57 +0x68
  github.com/alibaba/sentinel-golang/core/base.(*SentinelEntry).Exit()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/core/base/entry.go:62 +0x144
  main.main.func2()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/api/test/cb.go:67 +0x136

Previous write at 0x00c0000a0a00 by goroutine 10:
  github.com/alibaba/sentinel-golang/core/base.RefurbishTokenResult()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/core/base/result.go:118 +0x40a
  github.com/alibaba/sentinel-golang/core/base.(*SlotChain).Entry()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/core/base/slot_chain.go:145 +0x401
  github.com/alibaba/sentinel-golang/api.entry()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/api/api.go:114 +0x4c1
  github.com/alibaba/sentinel-golang/api.Entry()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/api/api.go:86 +0x19a
  main.main.func1()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/api/test/cb.go:47 +0x58

Goroutine 11 (running) created at:
  main.main()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/api/test/cb.go:58 +0x439

Goroutine 10 (running) created at:
  main.main()
      /Users/sczyh30/temp/to-review/sentinel-go-ximu/api/test/cb.go:45 +0x421
==================

Root cause is here:
Refer to: https://github.com/alibaba/sentinel-golang/commit/975debd7075d071473fcbcd17c3dbc3bd201a2da#diff-604f4192deea5adf9c159e5f88f64630R93


defer func() {
	if r != nil {
		base.RefurbishTokenResult(r)
	}
}()

r is the same Pointer with context.Output.LastResult. Sentinel refurbish the r. But When user call Exit() by manually, will call context.Output.LastResult(the refurbished TokenResult might be got by other goroutine), it causes DATA RACE.

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@louyuting louyuting marked this pull request as ready for review May 14, 2020 15:06
@louyuting louyuting marked this pull request as ready for review May 14, 2020 15:06
@louyuting louyuting requested a review from sczyh30 May 14, 2020 15:06
@louyuting louyuting added area/performance Issues or PRs related to runtime performance kind/bug Something isn't working labels May 14, 2020
if r.Status() == base.ResultStatusBlocked {
return r
result.DeepCopyFrom(r)
Copy link
Member

Choose a reason for hiding this comment

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

Why a copy is needed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not needed, maybe return r is enough

for _, rule := range rules {
passed, m := s.doCheckRule(rule)
if passed {
continue
}
return base.NewTokenResultBlockedWithCause(base.BlockTypeSystemFlow, base.BlockTypeSystemFlow.String(), rule, m)
result.ResetToBlockedWithCauseFrom(base.BlockTypeSystemFlow, base.BlockTypeSystemFlow.String(), rule, m)
Copy link
Member

Choose a reason for hiding this comment

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

Check result nil here? (in case previous customized slots cleared the result)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make sense

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #149 into master will decrease coverage by 0.76%.
The diff coverage is 33.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   43.48%   42.72%   -0.77%     
==========================================
  Files          67       67              
  Lines        2863     2926      +63     
==========================================
+ Hits         1245     1250       +5     
- Misses       1479     1537      +58     
  Partials      139      139              
Impacted Files Coverage Δ
core/base/block_error.go 10.00% <0.00%> (-18.58%) ⬇️
core/circuit_breaker/slot.go 0.00% <0.00%> (ø)
core/flow/slot.go 0.00% <0.00%> (ø)
core/flow/tc_default.go 0.00% <0.00%> (ø)
core/stat/stat_slot.go 0.00% <0.00%> (ø)
core/base/context.go 25.00% <23.07%> (-27.39%) ⬇️
core/base/result.go 29.35% <25.67%> (-8.98%) ⬇️
core/system/slot.go 56.60% <37.50%> (-3.82%) ⬇️
core/flow/tc_throttling.go 66.66% <50.00%> (ø)
core/circuit_breaker/circuit_breaker.go 46.21% <57.14%> (ø)
... and 3 more

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 975debd...bf97270. Read the comment docs.

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 b73ee5e into alibaba:master May 20, 2020
@sczyh30
Copy link
Member

sczyh30 commented May 20, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Issues or PRs related to runtime performance kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants