Skip to content

storage: avoid data race in TestSnapshotRaftLogLimit#38388

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/fixTestRace
Jun 25, 2019
Merged

storage: avoid data race in TestSnapshotRaftLogLimit#38388
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/fixTestRace

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Jun 25, 2019

I saw the following race in https://teamcity.cockroachdb.com/viewLog.html?buildId=1356733:

Race detected!

------- Stdout: -------
==================
Write at 0x00c001d9c2a8 by goroutine 63:
  github.com/cockroachdb/cockroach/pkg/storage.(*Replica).handleRaftReadyRaftMuLocked()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raft.go:637 +0x77a
  github.com/cockroachdb/cockroach/pkg/storage.(*Replica).handleRaftReady()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raft.go:386 +0x16a
  github.com/cockroachdb/cockroach/pkg/storage.(*Store).processReady()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:3742 +0x171
  github.com/cockroachdb/cockroach/pkg/storage.(*raftScheduler).worker()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/scheduler.go:227 +0x33e
  github.com/cockroachdb/cockroach/pkg/storage.(*raftScheduler).Start.func2()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/scheduler.go:161 +0x55
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunWorker.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:196 +0x15f

Previous read at 0x00c001d9c2a8 by goroutine 154:
  github.com/cockroachdb/cockroach/pkg/storage.TestSnapshotRaftLogLimit.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raftstorage.go:299 +0xa1
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:865 +0x163

I don't see an obvious reason why #38343 would have caused this, but maybe
it caused a new handleRaftReady iteration that allowed the race detector
to observe the issue. Either way, the test was broken and this fixes it.

Release note: None

@nvb nvb requested review from a team and tbg June 25, 2019 01:27
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jun 25, 2019

It turns out that this was the only use of replicaRaftStorage outside of replica_raftstorage.go and calls to newRaftConfig, so it really should have stuck out like a sore thumb.

I saw the following race in https://teamcity.cockroachdb.com/viewLog.html?buildId=1356733:

```
Race detected!

------- Stdout: -------
==================
Write at 0x00c001d9c2a8 by goroutine 63:
  github.com/cockroachdb/cockroach/pkg/storage.(*Replica).handleRaftReadyRaftMuLocked()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raft.go:637 +0x77a
  github.com/cockroachdb/cockroach/pkg/storage.(*Replica).handleRaftReady()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raft.go:386 +0x16a
  github.com/cockroachdb/cockroach/pkg/storage.(*Store).processReady()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:3742 +0x171
  github.com/cockroachdb/cockroach/pkg/storage.(*raftScheduler).worker()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/scheduler.go:227 +0x33e
  github.com/cockroachdb/cockroach/pkg/storage.(*raftScheduler).Start.func2()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/scheduler.go:161 +0x55
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunWorker.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:196 +0x15f

Previous read at 0x00c001d9c2a8 by goroutine 154:
  github.com/cockroachdb/cockroach/pkg/storage.TestSnapshotRaftLogLimit.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raftstorage.go:299 +0xa1
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:865 +0x163
```

I don't see an obvious reason why cockroachdb#38343 would have caused this, but maybe
it caused a new handleRaftReady iteration that allowed the race detector
to observe the issue. Either way, the test was broken and this fixes it.

Release note: None
@nvb nvb force-pushed the nvanbenschoten/fixTestRace branch from 0edaf51 to 500de1e Compare June 25, 2019 01:35
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Jun 25, 2019

bors r+

craig bot pushed a commit that referenced this pull request Jun 25, 2019
38388: storage: avoid data race in TestSnapshotRaftLogLimit r=nvanbenschoten a=nvanbenschoten

I saw the following race in https://teamcity.cockroachdb.com/viewLog.html?buildId=1356733:

```
Race detected!

------- Stdout: -------
==================
Write at 0x00c001d9c2a8 by goroutine 63:
  github.com/cockroachdb/cockroach/pkg/storage.(*Replica).handleRaftReadyRaftMuLocked()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raft.go:637 +0x77a
  github.com/cockroachdb/cockroach/pkg/storage.(*Replica).handleRaftReady()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raft.go:386 +0x16a
  github.com/cockroachdb/cockroach/pkg/storage.(*Store).processReady()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:3742 +0x171
  github.com/cockroachdb/cockroach/pkg/storage.(*raftScheduler).worker()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/scheduler.go:227 +0x33e
  github.com/cockroachdb/cockroach/pkg/storage.(*raftScheduler).Start.func2()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/scheduler.go:161 +0x55
  github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunWorker.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:196 +0x15f

Previous read at 0x00c001d9c2a8 by goroutine 154:
  github.com/cockroachdb/cockroach/pkg/storage.TestSnapshotRaftLogLimit.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_raftstorage.go:299 +0xa1
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:865 +0x163
```

I don't see an obvious reason why #38343 would have caused this, but maybe
it caused a new handleRaftReady iteration that allowed the race detector
to observe the issue. Either way, the test was broken and this fixes it.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 25, 2019

Build succeeded

@craig craig bot merged commit 500de1e into cockroachdb:master Jun 25, 2019
@nvb nvb deleted the nvanbenschoten/fixTestRace branch June 25, 2019 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants