sink: fix concurrent map access in sink manager #2249
Conversation
liuzix
added
status/ptal
ti-chi-bot
added
the
size/S
|
/run-leak-tests |
liuzix
changed the title
|
/run-leak-tests |
| @@ -92,8 +92,14 @@ func (s *managerSuite) TestManagerRandom(c *check.C) { | |||
| var wg sync.WaitGroup | |||
| tableSinks := make([]Sink, goroutineNum) | |||
| for i := 0; i < goroutineNum; i++ { | |||
| tableSinks[i] = manager.CreateTableSink(model.TableID(i), 0) | |||
| i := i | |||
There was a problem hiding this comment.
The variable i is deliberately shadowed to avoid programmer mistakes
There was a problem hiding this comment.
Because of closure? But with the same name kind of strange, imo
There was a problem hiding this comment.
It may seem odd to write
req := req
but it's legal and idiomatic in Go to do this. You get a fresh version of the variable with the same name, deliberately shadowing the loop variable locally but unique to each goroutine.
This way is widely used in Go, ref: https://golang.org/doc/effective_go
|
@ben1009: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: a256f08
|
ti-chi-bot
added
the
status/can-merge
|
/run-all-tests |
|
/run-leak-tests |
|
In response to a cherrypick label: new pull request created: #2298. |
|
In response to a cherrypick label: new pull request created: #2299. |
|
In response to a cherrypick label: new pull request created: #2300. |
What problem does this PR solve?
What is changed and how it works?
Check List
Tests
Related changes
Release note