Possible datarace at cacheShard.get

[ehnuje]

Is there any possibility of datarace call RUnlock before return by calling readEntry at the end of the function? How about call s.lock.RUnlock() by defer s.lock.RUnlock() to ensure lock is held until data copying is done?

func (s *cacheShard) get(key string, hashedKey uint64) ([]byte, error) {
	s.lock.RLock()
	itemIndex := s.hashmap[hashedKey]

	if itemIndex == 0 {
		s.lock.RUnlock()
		s.miss()
		return nil, ErrEntryNotFound
	}

	wrappedEntry, err := s.entries.Get(int(itemIndex))
	if err != nil {
		s.lock.RUnlock()
		s.miss()
		return nil, err
	}
	if entryKey := readKeyFromEntry(wrappedEntry); key != entryKey {
		if s.isVerbose {
			s.logger.Printf("Collision detected. Both %q and %q have the same hash %x", key, entryKey, hashedKey)
		}
		s.lock.RUnlock()
		s.collision()
		return nil, ErrEntryNotFound
	}
	s.lock.RUnlock() // call this function with defer
	s.hit()
	return readEntry(wrappedEntry), nil
}

[ehnuje]

I took a look into this code since bigCache returns a not nil but empty slice which is not expected to be returned.

[siennathesane]

For reasons not to use defer, please see #37.

[ehnuje]

Thanks for the explanation =)
If so, how about getting the result from the readEntry and then unlock manually like below?

s.hit()
result := readEntry(wrappedEntry)
s.lock.RUnlock()
return result, nil

[cristaloleg]

Good idea @ehnuje
Looks like this will solve an edge case.

[siennathesane]

I'm happy to take a look at any PRs you come up with @ehnuje :)

[ehnuje]

After check the latest code, it has been fixed by e24eb22
I’ve only checked my local code since we recently started using bigcache. Let me close this issue.