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: use atomic in the cache #2

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

hawkingrei
Copy link
Contributor

@hawkingrei hawkingrei commented Apr 5, 2022

Signed-off-by: Weizhen Wang [email protected]

we find the data race in the cache of ristretto when to test our cases.

==================
WARNING: DATA RACE
Write at 0x00c002c1a0d8 by goroutine 99:
  github.com/dgraph-io/ristretto.(*Cache).Close()
      /home/prow/go/pkg/mod/github.com/dgraph-io/[email protected]/cache.go:363 +0xe4
  github.com/pingcap/tidb/store/copr.(*Store).Close()
      /go/tidb/store/copr/store.go:97 +0x91
  github.com/pingcap/tidb/store/mockstore/mockstorage.(*mockStorage).Close()
      /go/tidb/store/mockstore/mockstorage/storage.go:116 +0x27
  github.com/pingcap/tidb/testkit.bootstrap.func1()
      /go/tidb/testkit/mockstore.go:57 +0x68
  runtime.deferreturn()
      /usr/local/go/src/runtime/panic.go:436 +0x32
  github.com/pingcap/failpoint.parseTerm()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/terms.go:149 +0x377
  github.com/pingcap/failpoint.parse()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/terms.go:126 +0xa8
  github.com/pingcap/failpoint.newTerms()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/terms.go:98 +0x50
  github.com/pingcap/failpoint.(*Failpoint).Enable()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/failpoint.go:54 +0x44
  github.com/pingcap/failpoint.(*Failpoints).Enable()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/failpoints.go:105 +0x276
  github.com/pingcap/failpoint.Enable()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/failpoints.go:225 +0x14f
  github.com/pingcap/tidb/executor/seqtest_test.TestParallelHashAggClose()
      /go/tidb/executor/seqtest/seq_executor_test.go:711 +0x150
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1486 +0x47
Previous read at 0x00c002c1a0d8 by goroutine 7:
  github.com/dgraph-io/ristretto.(*Cache).Get()
      /home/prow/go/pkg/mod/github.com/dgraph-io/[email protected]/cache.go:225 +0x4a
  github.com/pingcap/tidb/store/copr.(*coprCache).Get()
      /go/tidb/store/copr/coprocessor_cache.go:152 +0x84
  github.com/pingcap/tidb/store/copr.(*copIteratorWorker).handleTaskOnce()
      /go/tidb/store/copr/coprocessor.go:724 +0x744
  github.com/pingcap/tidb/store/copr.(*copIteratorWorker).handleTask()
      /go/tidb/store/copr/coprocessor.go:676 +0x1e5
  github.com/pingcap/tidb/store/copr.(*copIteratorWorker).run()
      /go/tidb/store/copr/coprocessor.go:418 +0x173
  github.com/pingcap/tidb/store/copr.(*copIterator).open.func1()
      /go/tidb/store/copr/coprocessor.go:450 +0x58
Goroutine 99 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1486 +0x724
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1839 +0x99
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1439 +0x213
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1837 +0x7e4
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1719 +0xa71
  go.uber.org/goleak.VerifyTestMain()
      /home/prow/go/pkg/mod/go.uber.org/[email protected]/testmain.go:53 +0x59
  github.com/pingcap/tidb/executor/seqtest.TestMain()
      /go/tidb/executor/seqtest/main_test.go:37 +0x430
  main.main()
      _testmain.go:125 +0x317
Goroutine 7 (finished) created at:
  github.com/pingcap/tidb/store/copr.(*copIterator).open()
      /go/tidb/store/copr/coprocessor.go:450 +0xc4
  github.com/pingcap/tidb/store/copr.(*CopClient).Send()
      /go/tidb/store/copr/coprocessor.go:143 +0x10c9
  github.com/pingcap/tidb/distsql.Select()
      /go/tidb/distsql/distsql.go:101 +0x8c3
  github.com/pingcap/tidb/distsql.SelectWithRuntimeStats()
      /go/tidb/distsql/distsql.go:150 +0xbb
  github.com/pingcap/tidb/executor.selectResultHook.SelectResult()
      /go/tidb/executor/table_reader.go:53 +0x1c5
  github.com/pingcap/tidb/executor.(*TableReaderExecutor).buildResp()
      /go/tidb/executor/table_reader.go:310 +0x69d
  github.com/pingcap/tidb/executor.(*TableReaderExecutor).Open()
      /go/tidb/executor/table_reader.go:210 +0x13f5
  github.com/pingcap/tidb/executor.(*baseExecutor).Open()
      /go/tidb/executor/executor.go:185 +0x6a9
  github.com/pingcap/tidb/executor.(*HashAggExec).Open()
      /go/tidb/executor/aggregate.go:309 +0xe6
  github.com/pingcap/tidb/executor.(*ExecStmt).Exec()
      /go/tidb/executor/adapter.go:407 +0x7f8
  github.com/pingcap/tidb/session.runStmt()
      /go/tidb/session/session.go:2017 +0x6cb
  github.com/pingcap/tidb/session.(*session).ExecuteStmt()
      /go/tidb/session/session.go:1894 +0xd3a
  github.com/pingcap/tidb/session.(*session).Execute()
      /go/tidb/session/session.go:1534 +0x478
  github.com/pingcap/tidb/executor/seqtest_test.TestParallelHashAggClose()
      /go/tidb/executor/seqtest/seq_executor_test.go:716 +0x21b
  github.com/pingcap/failpoint.parseTerm()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/terms.go:149 +0x377
  github.com/pingcap/failpoint.parse()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/terms.go:126 +0xa8
  github.com/pingcap/failpoint.newTerms()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/terms.go:98 +0x50
  github.com/pingcap/failpoint.(*Failpoint).Enable()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/failpoint.go:54 +0x44
  github.com/pingcap/failpoint.(*Failpoints).Enable()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/failpoints.go:105 +0x276
  github.com/pingcap/failpoint.Enable()
      /home/prow/go/pkg/mod/github.com/pingcap/[email protected]/failpoints.go:225 +0x14f
  github.com/pingcap/tidb/executor/seqtest_test.TestParallelHashAggClose()
      /go/tidb/executor/seqtest/seq_executor_test.go:711 +0x150
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1439 +0x213
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1486 +0x47
================== 

ref pingcap/tidb#33649


This change is Reviewable

Signed-off-by: Weizhen Wang <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Apr 5, 2022

CLA assistant check
All committers have signed the CLA.

@manishrjain manishrjain requested review from karlmcguire and removed request for manishrjain April 6, 2022 17:52
@manishrjain
Copy link
Contributor

Thanks, @hawkingrei . @karlmcguire would have a look.

@manishrjain manishrjain added the bug Something isn't working label Apr 6, 2022
@karlmcguire
Copy link
Contributor

Looks good to me! Wasn't sure about Uber's atomic library, but it performs just like standard atomic.Store/Load32 so I'm happy with using it for readability.

@karlmcguire karlmcguire merged commit a48e667 into outcaste-io:main Apr 6, 2022
@hawkingrei hawkingrei deleted the fix_data_race branch April 7, 2022 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants