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

expression: fix data race of rand function #11168

Merged
merged 3 commits into from
Jul 10, 2019
Merged

expression: fix data race of rand function #11168

merged 3 commits into from
Jul 10, 2019

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented Jul 10, 2019

What problem does this PR solve? 

WARNING: DATA RACE
Read at 0x00c000345508 by goroutine 81:
  math/rand.(*rngSource).Int63()
      /usr/lib/go/src/math/rand/rng.go:244 +0x8f
  github.com/pingcap/tidb/expression.(*builtinRandSig).evalReal()
      /usr/lib/go/src/math/rand/rand.go:85 +0x7a
  github.com/pingcap/tidb/expression.(*ScalarFunction).EvalReal()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/scalar_function.go:239 +0x73
  github.com/pingcap/tidb/expression.executeToReal()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/chunk_executor.go:143 +0x79
  github.com/pingcap/tidb/expression.evalOneColumn()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/chunk_executor.go:75 +0x2c0
  github.com/pingcap/tidb/expression.(*defaultEvaluator).run()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/evaluator.go:47 +0x664
  github.com/pingcap/tidb/expression.(*EvaluatorSuite).Run()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/evaluator.go:113 +0x113
  github.com/pingcap/tidb/executor.(*projectionWorker).run()
      /home/aliv/go/src/github.com/pingcap/tidb/executor/projection.go:356 +0x246

Previous write at 0x00c000345508 by goroutine 116:
  math/rand.(*rngSource).Int63()
      /usr/lib/go/src/math/rand/rng.go:244 +0xab
  github.com/pingcap/tidb/expression.(*builtinRandSig).evalReal()
      /usr/lib/go/src/math/rand/rand.go:85 +0x7a
  github.com/pingcap/tidb/expression.(*ScalarFunction).EvalReal()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/scalar_function.go:239 +0x73
  github.com/pingcap/tidb/expression.executeToReal()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/chunk_executor.go:143 +0x79
  github.com/pingcap/tidb/expression.evalOneColumn()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/chunk_executor.go:75 +0x2c0
  github.com/pingcap/tidb/expression.(*defaultEvaluator).run()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/evaluator.go:47 +0x664
  github.com/pingcap/tidb/expression.(*EvaluatorSuite).Run()
      /home/aliv/go/src/github.com/pingcap/tidb/expression/evaluator.go:113 +0x113
  github.com/pingcap/tidb/executor.(*projectionWorker).run()
      /home/aliv/go/src/github.com/pingcap/tidb/executor/projection.go:356 +0x246

What is changed and how it works?

Add a mutex for rand to prevent data race.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

winkyao
winkyao reviewed Jul 10, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

winoros
winoros reviewed Jul 10, 2019
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@alivxxx
Copy link
Contributor Author

alivxxx commented Jul 10, 2019

/run-all-tests

@alivxxx alivxxx added needs-cherry-pick-2.1 status/LGT2 Indicates that a PR has LGTM 2. labels Jul 10, 2019
XuHuaiyu
XuHuaiyu reviewed Jul 10, 2019
View reviewed changes
expression/builtin_math.go
@@ -988,19 +989,23 @@ func (c *randFunctionClass) getFunction(ctx sessionctx.Context, args []Expressio

type builtinRandSig struct {
baseBuiltinFunc
mu *sync.Mutex
Copy link
Contributor

Choose a reason for hiding this comment

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

All the projectWorkers will battle for this Mutex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and this is the only way to make it correct.

@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #11168 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11168   +/-   ##
===========================================
  Coverage   81.1123%   81.1123%           
===========================================
  Files           423        423           
  Lines         90382      90382           
===========================================
  Hits          73311      73311           
  Misses        11788      11788           
  Partials       5283       5283

@alivxxx
Copy link
Contributor Author

alivxxx commented Jul 10, 2019

/run-integration-ddl-test

@alivxxx alivxxx merged commit f409f0b into pingcap:master Jul 10, 2019
@alivxxx alivxxx deleted the data-race branch July 10, 2019 09:39
@alivxxx alivxxx changed the title expressio: fix data race of rand function expression: fix data race of rand function Jul 10, 2019
alivxxx added a commit to alivxxx/tidb that referenced this pull request Jul 10, 2019
@alivxxx
expression: fix data race of rand function (pingcap#11168)
5eb9065
@alivxxx alivxxx mentioned this pull request Jul 10, 2019
Merged
alivxxx added a commit to alivxxx/tidb that referenced this pull request Jul 10, 2019
@alivxxx
expression: fix data race of rand function (pingcap#11168)
878730d
@alivxxx alivxxx mentioned this pull request Jul 10, 2019
Merged
ngaut pushed a commit that referenced this pull request Jul 11, 2019
@alivxxx @ngaut
expression: fix data race of rand function (#11168) (#11169)
d1848b0
zz-jason pushed a commit that referenced this pull request Jul 11, 2019
@alivxxx @zz-jason
expression: fix data race of rand function (#11168) (#11170)
7cdce5b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winkyao winkyao winkyao left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@winoros winoros winoros approved these changes

@zz-jason zz-jason Awaiting requested review from zz-jason

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
component/expression status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alivxxx @winkyao @winoros @XuHuaiyu