diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index b79219d9..d9ded670 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -43,6 +43,13 @@ jobs: GO111MODULE: on run: go test -race -mod=readonly -count 2 ./... + # Run all consistenthash Fuzz tests for 30s with go 1.19 + - name: Fuzz Consistent-Hash + if: ${{matrix.goversion == '1.19'}} + env: + GO111MODULE: on + run: go test -fuzz=. -fuzztime=30s ./consistenthash + # Run all the tests with the paranoid linked-list checks enabled - name: Race Test Paranoid LinkedList env: diff --git a/consistenthash/consistenthash.go b/consistenthash/consistenthash.go index c45692aa..a9b8bc15 100644 --- a/consistenthash/consistenthash.go +++ b/consistenthash/consistenthash.go @@ -62,7 +62,19 @@ func (m *Map) Add(keys ...string) { m.keys[key] = struct{}{} for i := 0; i < m.segsPerKey; i++ { hash := m.hash([]byte(strconv.Itoa(i) + key)) - m.keyHashes = append(m.keyHashes, hash) + // If there's a collision on a "replica" (segment-boundary), we only want + // the entry that sorts latest to get inserted (not the last one we saw). + // + // It doesn't matter how we reconcile collisions (the smallest would work + // just as well), we just need it to be insertion-order independent so all + // instances converge on the same hashmap. + if extKey, ok := m.hashMap[hash]; !ok { + // Only add another member for this hash-value if there shouldn't be + // one there already. + m.keyHashes = append(m.keyHashes, hash) + } else if extKey >= key { + continue + } m.hashMap[hash] = key } } diff --git a/consistenthash/consistenthash_fuzz_test.go b/consistenthash/consistenthash_fuzz_test.go new file mode 100644 index 00000000..9bd1bc25 --- /dev/null +++ b/consistenthash/consistenthash_fuzz_test.go @@ -0,0 +1,61 @@ +//go:build go1.18 + +/* +Copyright 2022 Vimeo Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package consistenthash + +import ( + "math/rand" + "reflect" + "testing" +) + +func FuzzHashCollision(f *testing.F) { + hashFunc := func(d []byte) uint32 { + if len(d) < 2 { + return uint32(d[0]) + } + return uint32(d[0] + d[1]) + } + f.Fuzz(func(t *testing.T, in1, in2, in3, in4 string, seed int64, segments uint) { + // Skip anything with too many segments + if segments < 1 || segments > 1<<20 { + t.Skip() + } + src := rand.NewSource(seed) + r := rand.New(src) + input := [...]string{in1, in2, in3, in4} + r.Shuffle(len(input), func(i, j int) { input[i], input[j] = input[j], input[i] }) + + hash1 := New(int(segments), hashFunc) + hash1.Add(input[:]...) + + r.Shuffle(len(input), func(i, j int) { input[i], input[j] = input[j], input[i] }) + hash2 := New(int(segments), hashFunc) + hash2.Add(input[:]...) + + if !reflect.DeepEqual(hash1.hashMap, hash2.hashMap) { + t.Errorf("hash maps are not identical: %+v vs %+v", hash1.hashMap, hash2.hashMap) + } + if !reflect.DeepEqual(hash1.keys, hash2.keys) { + t.Errorf("hash keys are not identical: %+v vs %+v", hash1.keys, hash2.keys) + } + if !reflect.DeepEqual(hash1.keyHashes, hash2.keyHashes) { + t.Errorf("hash keys are not identical: %+v vs %+v", hash1.keys, hash2.keys) + } + }) +} diff --git a/consistenthash/consistenthash_test.go b/consistenthash/consistenthash_test.go index d9533f85..ea3d270b 100644 --- a/consistenthash/consistenthash_test.go +++ b/consistenthash/consistenthash_test.go @@ -65,6 +65,26 @@ func TestHashing(t *testing.T) { } +func TestHashCollision(t *testing.T) { + hashFunc := func(d []byte) uint32 { + return uint32(d[0]) + } + hash1 := New(1, hashFunc) + hash1.Add("Bill", "Bob", "Bonny") + hash2 := New(1, hashFunc) + hash2.Add("Bill", "Bonny", "Bob") + + t.Log(hash1.hashMap[uint32('0')], hash2.hashMap[uint32('0')]) + t.Logf("%+v", hash1.hashMap) + t.Logf("%v", hash1.keyHashes) + t.Logf("%+v", hash2.hashMap) + t.Logf("%v", hash2.keyHashes) + if hash1.hashMap[uint32('0')] != hash2.hashMap[uint32('0')] { + t.Errorf("inconsistent owner for hash %d: %s vs %s", 'B', + hash1.hashMap[uint32('B')], hash2.hashMap[uint32('B')]) + } +} + func TestConsistency(t *testing.T) { hash1 := New(1, nil) hash2 := New(1, nil)