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.

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

Fix map access panic #343

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Fix map access panic #343

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e36329
fix concurrent map access panic
stevenlanders Oct 30, 2023
0f6da88
avoid concurrent access to readset/iterateset
stevenlanders Oct 30, 2023
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
7 changes: 5 additions & 2 deletions store/multiversion/store.go
View file
Open in desktop
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
Loading