hashring icon indicating copy to clipboard operation
hashring copied to clipboard

Does Locate() need a Lock instead of an RLock?

Open jimrobinson opened this issue 5 years ago • 0 comments

Locate is acquiring a read lock, but it is calling getHash(hr.hash, []byte(key)):

// Locate returns the node for a given key
func (hr *HashRing) Locate(key string) (node string, err error) {
	hr.mu.RLock()
	defer hr.mu.RUnlock()

	if len(hr.idx) < 1 {
		return node, fmt.Errorf("no available nodes")
	}

	hkey, err := getHash(hr.hash, []byte(key))
	if err != nil {
		return node, fmt.Errorf("failed to fetch node: %v", err)
	}

	pos := sort.Search(len(hr.idx), func(i int) bool { return hr.idx[i] >= hkey })
	if pos == len(hr.idx) {
		return hr.nodes[hr.idx[0]], nil
	}
	return hr.nodes[hr.idx[pos]], nil
}

in getHash() we see it performing a hash reset:

// getHash returns uint32 hash
func getHash(hash hash.Hash32, key []byte) (uint32, error) {
	hash.Reset()
	_, err := hash.Write(key)
	if err != nil {
		return 0, err
	}

	return hash.Sum32(), nil
}

this appears to trigger the race detector:

Write at 0x00c0005000c8 by goroutine 85:
  hash/fnv.(*sum32a).Reset()
      /usr/local/go/src/hash/fnv/fnv.go:88 +0x3a
  github.com/vedhavyas/hashring.getHash()
      /home/jimr/proj/github/src/github.com/highwire/rstore/vendor/github.com/vedhavyas/hashring/hashring.go:54 +0x42
  github.com/vedhavyas/hashring.(*HashRing).Locate()
      /home/jimr/proj/github/src/github.com/highwire/rstore/vendor/github.com/vedhavyas/hashring/hashring.go:121 +0x17d
  github.com/highwire/rstore/peers.(*Peers).Resolve()
      /home/jimr/proj/github/src/github.com/highwire/rstore/peers/peers.go:181 +0x20b
  github.com/highwire/rstore/peers.BenchmarkPeersResolve.func1.1()
      /home/jimr/proj/github/src/github.com/highwire/rstore/peers/peers_test.go:104 +0xcb
  testing.(*B).RunParallel.func1()
      /usr/local/go/src/testing/benchmark.go:763 +0x182

Previous write at 0x00c0005000c8 by goroutine 82:
  hash/fnv.(*sum32a).Reset()
      /usr/local/go/src/hash/fnv/fnv.go:88 +0x3a
  github.com/vedhavyas/hashring.getHash()
      /home/jimr/proj/github/src/github.com/highwire/rstore/vendor/github.com/vedhavyas/hashring/hashring.go:54 +0x42
  github.com/vedhavyas/hashring.(*HashRing).Locate()
      /home/jimr/proj/github/src/github.com/highwire/rstore/vendor/github.com/vedhavyas/hashring/hashring.go:121 +0x17d
  github.com/highwire/rstore/peers.(*Peers).Resolve()
      /home/jimr/proj/github/src/github.com/highwire/rstore/peers/peers.go:181 +0x20b
  github.com/highwire/rstore/peers.BenchmarkPeersResolve.func1.1()
      /home/jimr/proj/github/src/github.com/highwire/rstore/peers/peers_test.go:104 +0xcb
  testing.(*B).RunParallel.func1()
      /usr/local/go/src/testing/benchmark.go:763 +0x182

jimrobinson avatar Jun 28 '20 11:06 jimrobinson