From 2c509dd3b1e443b4a106824e6459e912b03fe988 Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf <tobias.schottdorf@gmail.com> Date: Wed, 3 Aug 2016 06:06:11 -0400 Subject: [PATCH] storage: do not use non-authoritative Desc() After ec007326db720f9028f793438ea40a6ce1dc4da1, we were using a denormalized copy of the descriptor which is not protected by the same mutex in places where we shouldn't, resulting in subtle races. Change it back to use the authoritative one almost everywhere, except for informational callsites (such as stringifying a `Replica`). Fixes #8086. Fixes #8157. --- roachpb/errors.go | 2 +- storage/replica.go | 19 ++++++++++++++++--- storage/storagebase/state.pb.go | 2 +- storage/storagebase/state.proto | 2 +- storage/store_test.go | 16 ++++++++-------- 5 files changed, 27 insertions(+), 14 deletions(-) diff --git a/roachpb/errors.go b/roachpb/errors.go index e6e147c59723..4cba67818bcf 100644 --- a/roachpb/errors.go +++ b/roachpb/errors.go @@ -298,7 +298,7 @@ func NewRangeKeyMismatchError(start, end Key, desc *RangeDescriptor) *RangeKeyMi if desc != nil && !desc.IsInitialized() { // We must never send uninitialized ranges back to the client (nil // is fine) guard against regressions of #6027. - panic("descriptor is not initialized") + panic(fmt.Sprintf("descriptor is not initialized: %+v", desc)) } return &RangeKeyMismatchError{ RequestStartKey: start, diff --git a/storage/replica.go b/storage/replica.go index eef7218e11ef..2da122a6ce75 100644 --- a/storage/replica.go +++ b/storage/replica.go @@ -466,7 +466,7 @@ func (r *Replica) newReplicaInner(desc *roachpb.RangeDescriptor, clock *hlc.Cloc // String returns a string representation of the range. func (r *Replica) String() string { - desc := r.Desc() + desc := r.InformalDesc() return fmt.Sprintf("%s range=%d [%s-%s)", r.store, desc.RangeID, desc.StartKey, desc.EndKey) } @@ -703,8 +703,19 @@ func (r *Replica) isInitializedLocked() bool { return r.mu.state.Desc.IsInitialized() } -// Desc returns the range's descriptor. +// Desc returns the authoritative range descriptor, acquiring a replica lock in +// the process. func (r *Replica) Desc() *roachpb.RangeDescriptor { + r.mu.Lock() + defer r.mu.Unlock() + return r.mu.state.Desc +} + +// InformalDesc returns a cached copy of the authoritative desc which is kept +// in sync with the authoritative desc, though not atomically. This method +// does not acquire a lock and is to be used whenever a descriptor is needed +// for informational purposes. +func (r *Replica) InformalDesc() *roachpb.RangeDescriptor { return r.rangeDesc.Load().(*roachpb.RangeDescriptor) } @@ -737,8 +748,10 @@ func (r *Replica) setDescWithoutProcessUpdateLocked(desc *roachpb.RangeDescripto desc.RangeID, r.RangeID) } - r.mu.state.Desc = desc + // NB: If we used rangeDesc for anything but informational purposes, the + // order here would be crucial. r.rangeDesc.Store(desc) + r.mu.state.Desc = desc } // GetReplicaDescriptor returns the replica for this range from the range diff --git a/storage/storagebase/state.pb.go b/storage/storagebase/state.pb.go index e468a623aa76..922b88292cba 100644 --- a/storage/storagebase/state.pb.go +++ b/storage/storagebase/state.pb.go @@ -49,7 +49,7 @@ type ReplicaState struct { // The pointer may change, but the referenced RangeDescriptor struct itself // must be treated as immutable; it is leaked out of the lock. // - // Changes of the descriptor should normally go through one of the + // Changes of the descriptor should always go through one of the // (*Replica).setDesc* methods. Desc *cockroach_roachpb.RangeDescriptor `protobuf:"bytes,3,opt,name=desc" json:"desc,omitempty"` // The latest lease, if any. diff --git a/storage/storagebase/state.proto b/storage/storagebase/state.proto index c10de1baff72..8f4b2b24527b 100644 --- a/storage/storagebase/state.proto +++ b/storage/storagebase/state.proto @@ -37,7 +37,7 @@ message ReplicaState { // The pointer may change, but the referenced RangeDescriptor struct itself // must be treated as immutable; it is leaked out of the lock. // - // Changes of the descriptor should normally go through one of the + // Changes of the descriptor should always go through one of the // (*Replica).setDesc* methods. roachpb.RangeDescriptor desc = 3; // The latest lease, if any. diff --git a/storage/store_test.go b/storage/store_test.go index fabcaa51c4a5..c9cb5e5e8a76 100644 --- a/storage/store_test.go +++ b/storage/store_test.go @@ -1062,28 +1062,28 @@ func TestStoreReplicasByKey(t *testing.T) { r4 := splitTestRange(store, roachpb.RKey("X"), roachpb.RKey("ZZ"), t) if r := store.LookupReplica(roachpb.RKey("0"), nil); r != r0 { - t.Errorf("mismatched range %+v != %+v", r.Desc(), r0.Desc()) + t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r0.InformalDesc()) } if r := store.LookupReplica(roachpb.RKey("B"), nil); r != r1 { - t.Errorf("mismatched range %+v != %+v", r.Desc(), r1.Desc()) + t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r1.InformalDesc()) } if r := store.LookupReplica(roachpb.RKey("C"), nil); r != r2 { - t.Errorf("mismatched range %+v != %+v", r.Desc(), r2.Desc()) + t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r2.InformalDesc()) } if r := store.LookupReplica(roachpb.RKey("M"), nil); r != r2 { - t.Errorf("mismatched range %+v != %+v", r.Desc(), r2.Desc()) + t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r2.InformalDesc()) } if r := store.LookupReplica(roachpb.RKey("X"), nil); r != r3 { - t.Errorf("mismatched range %+v != %+v", r.Desc(), r3.Desc()) + t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r3.InformalDesc()) } if r := store.LookupReplica(roachpb.RKey("Z"), nil); r != r3 { - t.Errorf("mismatched range %+v != %+v", r.Desc(), r3.Desc()) + t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r3.InformalDesc()) } if r := store.LookupReplica(roachpb.RKey("ZZ"), nil); r != r4 { - t.Errorf("mismatched range %+v != %+v", r.Desc(), r4.Desc()) + t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r4.InformalDesc()) } if r := store.LookupReplica(roachpb.RKey("\xff\x00"), nil); r != r4 { - t.Errorf("mismatched range %+v != %+v", r.Desc(), r4.Desc()) + t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r4.InformalDesc()) } if store.LookupReplica(roachpb.RKeyMax, nil) != nil { t.Errorf("expected roachpb.KeyMax to not have an associated range")