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

ddl: fix concurrent access to `do.infoHandle` has a data race error #8287

Merged
merged 14 commits into from Nov 27, 2018

Conversation

@ciscoxll
Member

ciscoxll commented Nov 12, 2018

What problem does this PR solve?

WARNING: DATA RACE
Write at 0x00c0001d4c70 by goroutine 51:
  github.com/pingcap/tidb/ddl_test.(*testFailDBSuite).TestHalfwayCancelOperations()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/domain/domain.go:538 +0x1c80
  github.com/pingcap/tidb/ddl_test.(*testFailDBSuite).TestHalfwayCancelOperations()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/ddl/fail_db_test.go:125 +0x1adb
  github.com/pingcap/tidb/ddl_test.(*testFailDBSuite).TestHalfwayCancelOperations()

 github.com/pingcap/tidb/ddl_test.(*testFailDBSuite).TestHalfwayCancelOperations()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/ddl/fail_db_test.go:69 +0x70
  runtime.call32()
      /usr/local/go/src/runtime/asm_amd64.s:522 +0x3a
  reflect.Value.Call()
      /usr/local/go/src/reflect/value.go:308 +0xc0
  github.com/pingcap/check.(*suiteRunner).forkTest.func1()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20171206051426-1c287c953996/check.go:798 +0xa04
  github.com/pingcap/check.(*suiteRunner).forkCall.func1()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/pkg/mod/github.com/pingcap/check@v0.0.0-20171206051426-1c287c953996/check.go:692 +0x89

Previous read at 0x00c0001d4c70 by goroutine 179:
  github.com/pingcap/tidb/domain.(*Domain).Reload()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/domain/domain.go:304 +0x151
  github.com/pingcap/tidb/domain.(*Domain).loadSchemaInLoop()
      /home/jenkins/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/domain/domain.go:401 +0x7f1

What is changed and how it works?

  • Data race error when concurrently accessing do.infoHandle in this PR test.
  • So here is the atomic pointer operation on do.infoHandle.

Check List

Tests

  • Unit test

This change is Reviewable

@ciscoxll ciscoxll changed the title from Fix concurrent access to `do.infoHandle` has a data race error to ddl: fix concurrent access to `do.infoHandle` has a data race error Nov 12, 2018

@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 12, 2018

@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 12, 2018

/run-all-tests

1 similar comment
@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 13, 2018

/run-all-tests

@@ -535,7 +535,8 @@ func NewDomain(store kv.Storage, ddlLease time.Duration, statsLease time.Duratio
// ResetHandle resets the domain's infoschema handle. It is used for testing.
func (do *Domain) ResetHandle(store kv.Storage) {

This comment has been minimized.

@tiancaiamao

tiancaiamao Nov 13, 2018

Contributor

I think it's better to remove this function, and call do.Reload() instead
@zimulala @ciscoxll

This comment has been minimized.

@winkyao

winkyao Nov 13, 2018

Member

Call do.Reload still has data race.

This comment has been minimized.

@zimulala

zimulala Nov 13, 2018

Member

This function is used to reset the handle. Reload can't do it.

This comment has been minimized.

@tiancaiamao

tiancaiamao Nov 13, 2018

Contributor

So why it's needed to reset the handle? @zimulala
do.Reload uses mutex.Lock @winkyao

This comment has been minimized.

@zimulala

zimulala Nov 13, 2018

Member

It's used for testing. You can see it in "TestHalfwayCancelOperations". @tiancaiamao

This comment has been minimized.

@zimulala

This comment has been minimized.

@winkyao

winkyao Nov 15, 2018

Member

@zimulala What do you mean?

This comment has been minimized.

@ciscoxll

ciscoxll Nov 22, 2018

Member

@zimulala @tiancaiamao Are we going to continue using s.dom.ResetHandle?

This comment has been minimized.

@zimulala

zimulala Nov 22, 2018

Member

@winkyao @ciscoxll
I mean we can change test. Maybe we can remove ResetHandle.
At most, the test results are not so good.

This comment has been minimized.

@ciscoxll

ciscoxll Nov 22, 2018

Member

@zimulala modified test failed to pass.

@ciscoxll ciscoxll force-pushed the ciscoxll:fix-data-race branch from 36ad682 to 35bbf56 Nov 13, 2018

@pingcap pingcap deleted a comment from winkyao Nov 13, 2018

@ciscoxll ciscoxll force-pushed the ciscoxll:fix-data-race branch from 6b02ccf to ce4d765 Nov 13, 2018

Show resolved Hide resolved domain/domain.go Outdated

@ciscoxll ciscoxll force-pushed the ciscoxll:fix-data-race branch from 8bc3405 to 435ebc5 Nov 13, 2018

ciscoxll added some commits Nov 13, 2018

@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 13, 2018

/run-all-tests

// InfoSchema gets information schema from domain.
func (do *Domain) InfoSchema() infoschema.InfoSchema {
return do.infoHandle.Get()

This comment has been minimized.

@winkyao

winkyao Nov 14, 2018

Member

All the read at do.infoHandle need to be protected.

This comment has been minimized.

@ciscoxll

ciscoxll added some commits Nov 14, 2018

@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 14, 2018

/run-all-tests

@winkyao

This comment has been minimized.

Member

winkyao commented Nov 14, 2018

@XuHuaiyu Will fix the union test failed, @ciscoxll you can just use the command /run-integration-common-test, to rerun the common-test

@winkyao

This comment has been minimized.

Member

winkyao commented Nov 14, 2018

/run-integration-common-test

@XuHuaiyu

This comment has been minimized.

Contributor

XuHuaiyu commented Nov 14, 2018

/run-common-test tidb-test=pr/649
/run-integration-common-test tidb-test=pr/649

@winkyao

This comment has been minimized.

Member

winkyao commented Nov 14, 2018

This PR need to be cherry-pick to 2.1 and 2.0

@tiancaiamao

This comment has been minimized.

Contributor

tiancaiamao commented Nov 16, 2018

@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 19, 2018

@ciscoxll ciscoxll force-pushed the ciscoxll:fix-data-race branch from a79fad4 to 1cbd0a3 Nov 26, 2018

ciscoxll added some commits Nov 26, 2018

@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 26, 2018

/run-all-tests

1 similar comment
@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 27, 2018

/run-all-tests

Show resolved Hide resolved ddl/fail_db_test.go Outdated

@ciscoxll ciscoxll force-pushed the ciscoxll:fix-data-race branch from efb6c71 to c46e366 Nov 27, 2018

@ciscoxll

This comment has been minimized.

Member

ciscoxll commented Nov 27, 2018

/run-all-tests

@winkyao

This comment has been minimized.

Member

winkyao commented Nov 27, 2018

/run-common-test tidb-test=pr/668

@winkyao

This comment has been minimized.

Member

winkyao commented Nov 27, 2018

/run-integration-common-test tidb-test=pr/668

Show resolved Hide resolved ddl/fail_db_test.go Outdated
Show resolved Hide resolved ddl/fail_db_test.go Outdated

@ciscoxll ciscoxll force-pushed the ciscoxll:fix-data-race branch from 77e25cc to 74ccab6 Nov 27, 2018

@crazycs520 crazycs520 added status/LGT2 and removed status/LGT1 labels Nov 27, 2018

@tiancaiamao

This comment has been minimized.

Contributor

tiancaiamao commented Nov 27, 2018

LGTM

@ciscoxll ciscoxll merged commit e5dc251 into pingcap:master Nov 27, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@ciscoxll ciscoxll deleted the ciscoxll:fix-data-race branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment