Skip to content

fix(shard): defer shard lock unlock so panics inside eviction do not strand writers #421

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.

Sign up for GitHub

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

Open
SAY-5 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(shard): defer shard lock unlock so panics inside eviction do not strand writers #421

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions shard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,14 @@ func (s *cacheShard) set(key string, hashedKey uint64, entry []byte) error {
currentTimestamp := uint64(s.clock.Epoch())

s.lock.Lock()
// Hold the shard lock with defer so that a panic deeper in
// readEntry / onRemove / providedOnRemoveWithReason (e.g. a
// makeslice-len-out-of-range from a corrupted entry) does not
// leave the lock permanently held. Previously, a panic during
// onEvict would tear down the goroutine before the manual
// Unlock below could run, and every subsequent writer on the
// same shard blocked forever (#401).
defer s.lock.Unlock()

if previousIndex := s.hashmap[hashedKey]; previousIndex != 0 {
if previousEntry, err := s.entries.Get(int(previousIndex)); err == nil {
Expand All @@ -141,11 +149,9 @@ func (s *cacheShard) set(key string, hashedKey uint64, entry []byte) error {
for {
if index, err := s.entries.Push(w); err == nil {
s.hashmap[hashedKey] = uint64(index)
s.lock.Unlock()
return nil
}
if s.removeOldestEntry(NoSpace) != nil {
s.lock.Unlock()
return errors.New("entry is bigger than max shard size")
}
}
Expand Down Expand Up @@ -199,26 +205,25 @@ func (s *cacheShard) setWrappedEntryWithoutLock(currentTimestamp uint64, w []byt

func (s *cacheShard) append(key string, hashedKey uint64, entry []byte) error {
s.lock.Lock()
// Same lock-hygiene fix as set(): hold the lock across the full
// body so a panic deeper in readEntry / onEvict / etc. cannot
// strand the shard in a permanently locked state (#401).
defer s.lock.Unlock()

wrappedEntry, err := s.getValidWrapEntry(key, hashedKey)

if err == ErrEntryNotFound {
err = s.addNewWithoutLock(key, hashedKey, entry)
s.lock.Unlock()
return err
return s.addNewWithoutLock(key, hashedKey, entry)
}
if err != nil {
s.lock.Unlock()
return err
}

currentTimestamp := uint64(s.clock.Epoch())

w := appendToWrappedEntry(currentTimestamp, wrappedEntry, entry, &s.entryBuffer)

err = s.setWrappedEntryWithoutLock(currentTimestamp, w, hashedKey)
s.lock.Unlock()

return err
return s.setWrappedEntryWithoutLock(currentTimestamp, w, hashedKey)
}

func (s *cacheShard) del(hashedKey uint64) error {
Expand Down