Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jimrobinson opened this issue Jun 28, 2020 · 0 comments
Open

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

jimrobinson opened this issue Jun 28, 2020 · 0 comments

Comments

@jimrobinson
Copy link

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
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

No branches or pull requests

1 participant