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

Fix map access panic #343

Merged
merged 2 commits into from
Oct 30, 2023
Merged
Changes from all commits
Commits
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
45 changes: 33 additions & 12 deletions store/multiversion/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ func (s *Store) SetEstimatedWriteset(index int, incarnation int, writeset WriteS
s.txWritesetKeys[index] = writeSetKeys
}

// GetWritesetKeys implements MultiVersionStore.
// GetAllWritesetKeys implements MultiVersionStore.
func (s *Store) GetAllWritesetKeys() map[int][]string {
s.mtx.RLock()
defer s.mtx.RUnlock()
Expand Down Expand Up @@ -243,10 +243,13 @@ func (s *Store) GetIterateset(index int) Iterateset {

// CollectIteratorItems implements MultiVersionStore. It will return a memDB containing all of the keys present in the multiversion store within the iteration range prior to (exclusive of) the index.
func (s *Store) CollectIteratorItems(index int) *db.MemDB {
s.mtx.RLock()
defer s.mtx.RUnlock()

sortedItems := db.NewMemDB()

// get all writeset keys prior to index
keys := s.GetAllWritesetKeys()
keys := s.txWritesetKeys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will acquire the lock for longer right? whereas none of the below logic actually invokes resources protected by the lock? does it make sense to do this way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meant to point to lines 246/247 where lock is acquired

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Go, maps are always returned by reference, not by value. This means returning a reference via GetAllWritesetKeys doesn't give us a copy, and doesn't do anything to protect the map. The concurrent-access here is the keys[i] moment.

for i := 0; i < index; i++ {
indexedWriteset, ok := keys[i]
if !ok {
Expand Down Expand Up @@ -316,21 +319,27 @@ func (s *Store) validateIterator(index int, tracker iterationTracker) bool {
}
}

// TODO: do we want to return bool + []int where bool indicates whether it was valid and then []int indicates only ones for which we need to wait due to estimates? - yes i think so?
func (s *Store) ValidateTransactionState(index int) (bool, []int) {
defer telemetry.MeasureSince(time.Now(), "store", "mvs", "validate")
conflictSet := map[int]struct{}{}
valid := true
func (s *Store) checkIteratorAtIndex(index int) bool {
s.mtx.RLock()
defer s.mtx.RUnlock()

// TODO: can we parallelize for all iterators?
iterateset := s.GetIterateset(index)
valid := true
iterateset := s.txIterateSets[index]
for _, iterationTracker := range iterateset {
iteratorValid := s.validateIterator(index, iterationTracker)
valid = valid && iteratorValid
}
return valid
}

func (s *Store) checkReadsetAtIndex(index int) (bool, []int) {
s.mtx.RLock()
defer s.mtx.RUnlock()

conflictSet := make(map[int]struct{})
readset := s.txReadSets[index]
valid := true

// validate readset
readset := s.GetReadset(index)
// iterate over readset and check if the value is the same as the latest value relateive to txIndex in the multiversion store
for key, value := range readset {
// get the latest value from the multiversion store
Expand All @@ -357,16 +366,28 @@ func (s *Store) ValidateTransactionState(index int) (bool, []int) {
}
}

// convert conflictset into sorted indices
conflictIndices := make([]int, 0, len(conflictSet))
for index := range conflictSet {
conflictIndices = append(conflictIndices, index)
}

sort.Ints(conflictIndices)

return valid, conflictIndices
}

// TODO: do we want to return bool + []int where bool indicates whether it was valid and then []int indicates only ones for which we need to wait due to estimates? - yes i think so?
func (s *Store) ValidateTransactionState(index int) (bool, []int) {
defer telemetry.MeasureSince(time.Now(), "store", "mvs", "validate")

// TODO: can we parallelize for all iterators?
iteratorValid := s.checkIteratorAtIndex(index)

readsetValid, conflictIndices := s.checkReadsetAtIndex(index)

return iteratorValid && readsetValid, conflictIndices
}

func (s *Store) WriteLatestToStore() {
s.mtx.Lock()
defer s.mtx.Unlock()
Expand Down
Loading