Skip to content

Conversation

@nvb
Copy link
Contributor

@nvb nvb commented Aug 12, 2019

This mirrors the AssertHeld methods we have on these mutexes and extends them for read locks. These serve as both a useful assertion and good documentation (because they compile away for real builds).

#39119 reminded me that I had been sitting on this commit since June as part of a larger change that won't be making it in any time soon.

Release note: None

@nvb nvb requested a review from ajwerner August 12, 2019 22:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/util/syncutil/mutex_sync_race_test.go, line 70 at r1 (raw file):

	m.Unlock()

	func() {

If you feel like adopting require this could be:

require.PanicsWithValue(t, "mutex is not read locked", m.AssertRHeld)

@nvb nvb force-pushed the nvanbenschoten/rmuHeld branch from a630d8a to 9ba111c Compare August 13, 2019 15:00
Copy link
Contributor Author

@nvb nvb left a comment

Choose a reason for hiding this comment

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

bors r=ajwerner

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)


pkg/util/syncutil/mutex_sync_race_test.go, line 70 at r1 (raw file):

Previously, ajwerner wrote…

If you feel like adopting require this could be:

require.PanicsWithValue(t, "mutex is not read locked", m.AssertRHeld)

Nice! Done.

@craig
Copy link
Contributor

craig bot commented Aug 13, 2019

Build failed (retrying...)

@nvb
Copy link
Contributor Author

nvb commented Aug 13, 2019

bors r-

Write at 0x00c000b2cdfc by goroutine 178:
  sync/atomic.AddInt32()
      /usr/local/go/src/runtime/race_amd64.s:269 +0xb
  github.com/cockroachdb/cockroach/pkg/util/syncutil.(*RWMutex).RLock()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/syncutil/mutex_sync_race.go:75 +0x54
  github.com/cockroachdb/cockroach/pkg/config.(*SystemConfig).getZoneEntry()
      /go/src/github.com/cockroachdb/cockroach/pkg/config/system.go:299 +0x5e
  github.com/cockroachdb/cockroach/pkg/config.(*SystemConfig).getZoneConfigForKey()
      /go/src/github.com/cockroachdb/cockroach/pkg/config/system.go:334 +0x5e
  github.com/cockroachdb/cockroach/pkg/config.(*SystemConfig).GetZoneConfigForKey()
      /go/src/github.com/cockroachdb/cockroach/pkg/config/system.go:279 +0x159
  github.com/cockroachdb/cockroach/pkg/storage.(*Store).systemGossipUpdate.func2()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:1698 +0xf6
  github.com/cockroachdb/cockroach/pkg/storage.(*storeReplicaVisitor).Visit()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:350 +0x2e2
  github.com/cockroachdb/cockroach/pkg/storage.(*Store).systemGossipUpdate()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:1696 +0x23c
  github.com/cockroachdb/cockroach/pkg/storage.(*Store).Start.func2()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/store.go:1409 +0x95
  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 0x00c000b2cdfc by goroutine 156:
  reflect.typedmemmove()
      /usr/local/go/src/runtime/mbarrier.go:177 +0x0
  reflect.packEface()
      /usr/local/go/src/reflect/value.go:119 +0x104
  reflect.valueInterface()
      /usr/local/go/src/reflect/value.go:1009 +0x170
  reflect.deepValueEqual()
      /usr/local/go/src/reflect/deepequal.go:132 +0xa61
  reflect.deepValueEqual()
      /usr/local/go/src/reflect/deepequal.go:101 +0xc7a
  reflect.deepValueEqual()
      /usr/local/go/src/reflect/deepequal.go:101 +0xc7a
  reflect.deepValueEqual()
      /usr/local/go/src/reflect/deepequal.go:101 +0xc7a
  reflect.deepValueEqual()
      /usr/local/go/src/reflect/deepequal.go:98 +0x1866
  reflect.DeepEqual()
      /usr/local/go/src/reflect/deepequal.go:196 +0x2f1
  github.com/cockroachdb/cockroach/pkg/storage_test.TestStoreGossipSystemData.func4()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/client_lease_test.go:208 +0x1f5
  github.com/cockroachdb/cockroach/pkg/testutils.SucceedsSoonError.func1()
      /go/src/github.com/cockroachdb/cockroach/pkg/testutils/soon.go:48 +0x52
  github.com/cockroachdb/cockroach/pkg/util/retry.ForDuration()
      /go/src/github.com/cockroachdb/cockroach/pkg/util/retry/retry.go:184 +0xf4
  github.com/cockroachdb/cockroach/pkg/testutils.SucceedsSoonError()
      /go/src/github.com/cockroachdb/cockroach/pkg/testutils/soon.go:54 +0x11a
  github.com/cockroachdb/cockroach/pkg/testutils.SucceedsSoon()
      /go/src/github.com/cockroachdb/cockroach/pkg/testutils/soon.go:34 +0x59
  github.com/cockroachdb/cockroach/pkg/storage_test.TestStoreGossipSystemData()
      /go/src/github.com/cockroachdb/cockroach/pkg/storage/client_lease_test.go:207 +0xbcf
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:865 +0x163

@craig
Copy link
Contributor

craig bot commented Aug 13, 2019

Canceled

This mirrors the AssertHeld methods we have on these mutexes and extends
them for read locks. These serve as both a useful assertion and good
documentation (because they compile away for real builds).

Release note: None
@nvb
Copy link
Contributor Author

nvb commented Aug 13, 2019

bors r+

craig bot pushed a commit that referenced this pull request Aug 13, 2019
39609: syncutil: add AssertRHeld methods to RWMutex implementations r=nvanbenschoten a=nvanbenschoten

This mirrors the AssertHeld methods we have on these mutexes and extends them for read locks. These serve as both a useful assertion and good documentation (because they compile away for real builds).

#39119 reminded me that I had been sitting on this commit since June as part of a larger change that won't be making it in any time soon.

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 13, 2019

Build succeeded

@craig craig bot merged commit 86241f9 into cockroachdb:master Aug 13, 2019
@nvb nvb deleted the nvanbenschoten/rmuHeld branch September 5, 2019 17:40
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