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.

Sign up for GitHub

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

Jump to bottom

store: use RLock when reading snapshot.replicaRead snapshot.taskID (#21627) #21632

Merged
merged 2 commits into from
Dec 10, 2020
Merged

store: use RLock when reading snapshot.replicaRead snapshot.taskID (#21627) #21632

merged 2 commits into from
Dec 10, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #21627 to release-4.0

What problem does this PR solve?

Problem Summary:

What is changed and how it works?

Fix data race:

[2020-12-09T14:20:41.627Z] ==================
[2020-12-09T14:20:41.627Z] WARNING: DATA RACE
[2020-12-09T14:20:41.627Z] Write at 0x00c0758c2fd0 by goroutine 940:
[2020-12-09T14:20:41.627Z]   github.com/pingcap/tidb/store/tikv.(*tikvSnapshot).SetOption()
[2020-12-09T14:20:41.627Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/store/tikv/snapshot.go:514 +0x457
[2020-12-09T14:20:41.627Z]   github.com/pingcap/tidb/executor.(*BatchPointGetExec).Open()
[2020-12-09T14:20:41.627Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/batch_point_get.go:115 +0x3dc
[2020-12-09T14:20:41.627Z]   github.com/pingcap/tidb/executor.(*baseExecutor).Open()
[2020-12-09T14:20:41.627Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:160 +0xa9
[2020-12-09T14:20:41.627Z]   github.com/pingcap/tidb/executor.(*ProjectionExec).Open()
[2020-12-09T14:20:41.627Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/projection.go:85 +0x52
[2020-12-09T14:20:41.627Z]   github.com/pingcap/tidb/executor.(*UnionExec).resultPuller()
[2020-12-09T14:20:41.627Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:1523 +0x9b0
[2020-12-09T14:20:41.628Z] 
[2020-12-09T14:20:41.628Z] Previous read at 0x00c0758c2fd0 by goroutine 1103:
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/store/tikv.(*tikvSnapshot).batchGetSingleRegion()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/store/tikv/snapshot.go:289 +0x928
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/store/tikv.(*tikvSnapshot).batchGetKeysByRegions()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/store/tikv/snapshot.go:243 +0xa3e
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/store/tikv.(*tikvSnapshot).BatchGet()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/store/tikv/snapshot.go:153 +0x72d
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/kv.(*BufferBatchGetter).BatchGet()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/kv/utils.go:102 +0x99e
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*BatchPointGetExec).initialize()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/batch_point_get.go:340 +0xa39
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*BatchPointGetExec).Next()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/batch_point_get.go:143 +0x819
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.Next()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:278 +0x27d
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*UnionExec).resultPuller()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:1540 +0x4e8
[2020-12-09T14:20:41.628Z] 
[2020-12-09T14:20:41.628Z] Goroutine 940 (running) created at:
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*UnionExec).initialize()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:1490 +0x548
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*UnionExec).Next()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:1566 +0x520
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.Next()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:278 +0x27d
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*recordSet).Next()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/adapter.go:129 +0x110
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/session.(*execStmtResult).Next()
[2020-12-09T14:20:41.628Z]       <autogenerated>:1 +0x84
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/session.GetRows4Test()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/session/tidb.go:287 +0x35a
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/session.ResultSetToStringSlice()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/session/tidb.go:305 +0xb8
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/util/testkit.(*TestKit).ResultSetToResultWithCtx()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:321 +0xa8
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/util/testkit.(*TestKit).MustQuery()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:316 +0x4e4
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor_test.(*testPointGetSuite).TestClusterIndexCBOPointGet()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/point_get_test.go:590 +0x4b4
[2020-12-09T14:20:41.628Z]   runtime.call32()
[2020-12-09T14:20:41.628Z]       /usr/local/go/src/runtime/asm_amd64.s:539 +0x3a
[2020-12-09T14:20:41.628Z]   reflect.Value.Call()
[2020-12-09T14:20:41.628Z]       /usr/local/go/src/reflect/value.go:321 +0xd3
[2020-12-09T14:20:41.628Z]   github.com/pingcap/check.(*suiteRunner).forkTest.func1()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/[email protected]/check.go:850 +0x9aa
[2020-12-09T14:20:41.628Z]   github.com/pingcap/check.(*suiteRunner).forkCall.func1()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/[email protected]/check.go:739 +0x113
[2020-12-09T14:20:41.628Z] 
[2020-12-09T14:20:41.628Z] Goroutine 1103 (running) created at:
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*UnionExec).initialize()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:1490 +0x548
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*UnionExec).Next()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:1566 +0x520
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.Next()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/executor.go:278 +0x27d
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor.(*recordSet).Next()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/adapter.go:129 +0x110
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/session.(*execStmtResult).Next()
[2020-12-09T14:20:41.628Z]       <autogenerated>:1 +0x84
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/session.GetRows4Test()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/session/tidb.go:287 +0x35a
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/session.ResultSetToStringSlice()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/session/tidb.go:305 +0xb8
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/util/testkit.(*TestKit).ResultSetToResultWithCtx()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:321 +0xa8
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/util/testkit.(*TestKit).MustQuery()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:316 +0x4e4
[2020-12-09T14:20:41.628Z]   github.com/pingcap/tidb/executor_test.(*testPointGetSuite).TestClusterIndexCBOPointGet()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/executor/point_get_test.go:590 +0x4b4
[2020-12-09T14:20:41.628Z]   runtime.call32()
[2020-12-09T14:20:41.628Z]       /usr/local/go/src/runtime/asm_amd64.s:539 +0x3a
[2020-12-09T14:20:41.628Z]   reflect.Value.Call()
[2020-12-09T14:20:41.628Z]       /usr/local/go/src/reflect/value.go:321 +0xd3
[2020-12-09T14:20:41.628Z]   github.com/pingcap/check.(*suiteRunner).forkTest.func1()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/[email protected]/check.go:850 +0x9aa
[2020-12-09T14:20:41.628Z]   github.com/pingcap/check.(*suiteRunner).forkCall.func1()
[2020-12-09T14:20:41.628Z]       /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/[email protected]/check.go:739 +0x113
[2020-12-09T14:20:41.628Z] ==================

What's Changed:
After #21561, replicaRead and taskID will be written parallelly, we need to add RLock when reading them to avoid write-read data race.

How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • No code

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • No release note.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot mentioned this pull request Dec 10, 2020
Merged
@ti-srebot ti-srebot requested a review from bobotu December 10, 2020 05:03
@ti-srebot ti-srebot added this to the 4.0.0 milestone Dec 10, 2020
@ti-srebot
Copy link
Contributor Author

@XuHuaiyu you're already a collaborator in bot's repo.

@XuHuaiyu
Copy link
Contributor

/run-all-tests

bobotu
bobotu reviewed Dec 10, 2020
View reviewed changes
Copy link
Contributor

@bobotu bobotu left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 10, 2020
@sticnarf
Copy link
Contributor

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 10, 2020
@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Dec 10, 2020
@jebter
Copy link

jebter commented Dec 10, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 10, 2020
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot merged commit a62d379 into pingcap:release-4.0 Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobotu bobotu bobotu left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

Assignees

@XuHuaiyu XuHuaiyu

Labels
component/store component/test status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug. type/4.0-cherry-pick
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ti-srebot @XuHuaiyu @sticnarf @jebter @bobotu