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

Jump to bottom

refactor: make cachekv store thread-safe again #14378

Merged
merged 10 commits into from
Dec 21, 2022
Merged

refactor: make cachekv store thread-safe again #14378

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
eb08ce1
refactor: make cachekv store thread-safe again
yihuang Dec 21, 2022
1757999
channgelog
yihuang Dec 21, 2022
b8e66ff
fix lint
yihuang Dec 21, 2022
10ff1cb
Merge branch 'main' into thread-safe-cachekv
yihuang Dec 21, 2022
3d5a52f
Update CHANGELOG.md
yihuang Dec 21, 2022
e36ff12
Update store/cachekv/internal/btree.go
yihuang Dec 21, 2022
a321063
better interface
yihuang Dec 21, 2022
354b208
make newMemIterator private again
yihuang Dec 21, 2022
51bae7b
Merge branch 'main' into thread-safe-cachekv
yihuang Dec 21, 2022
90f831d
fix lint
yihuang Dec 21, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/auth/vesting) [#13502](https://github.com/cosmos/cosmos-sdk/pull/13502) Add Amino Msg registration for `MsgCreatePeriodicVestingAccount`.
* (x/auth)[#13780](https://github.com/cosmos/cosmos-sdk/pull/13780) `id` (type of int64) in `AccountAddressByID` grpc query is now deprecated, update to account-id(type of uint64) to use `AccountAddressByID`.
* (x/group) [#13876](https://github.com/cosmos/cosmos-sdk/pull/13876) Fix group MinExecutionPeriod that is checked on execution now, instead of voting period end.
* (store) [#](https://github.com/cosmos/cosmos-sdk/pull/14378) make cachekv store thread-safe again, cleanup iteration and deletion logic, iteration is on strictly isolated view now, which is breaking from previous behavior.
yihuang marked this conversation as resolved.
Show resolved Hide resolved

### API Breaking Changes

Expand Down
38 changes: 23 additions & 15 deletions store/cachekv/internal/btree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"errors"

"github.com/cosmos/cosmos-sdk/store/types"
"github.com/tidwall/btree"
)

Expand All @@ -21,46 +22,53 @@ var errKeyEmpty = errors.New("key cannot be empty")
//
// We choose tidwall/btree over google/btree here because it provides API to implement step iterator directly.
type BTree struct {
tree btree.BTreeG[item]
tree *btree.BTreeG[item]
}

// NewBTree creates a wrapper around `btree.BTreeG`.
func NewBTree() *BTree {
return &BTree{tree: *btree.NewBTreeGOptions(byKeys, btree.Options{
Degree: bTreeDegree,
// Contract: cachekv store must not be called concurrently
NoLocks: true,
func NewBTree() BTree {
return BTree{tree: btree.NewBTreeGOptions(byKeys, btree.Options{
Degree: bTreeDegree,
NoLocks: false,
})}
}
yihuang marked this conversation as resolved.
Show resolved Hide resolved

func (bt *BTree) Set(key, value []byte) {
func (bt BTree) Set(key, value []byte) {
bt.tree.Set(newItem(key, value))
}

func (bt *BTree) Get(key []byte) []byte {
func (bt BTree) Get(key []byte) []byte {
i, found := bt.tree.Get(newItem(key, nil))
if !found {
return nil
}
return i.value
}

func (bt *BTree) Delete(key []byte) {
func (bt BTree) Delete(key []byte) {
bt.tree.Delete(newItem(key, nil))
}

func (bt *BTree) Iterator(start, end []byte) (*memIterator, error) { //nolint:revive
func (bt BTree) Iterator(start, end []byte) types.Iterator {
if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
return nil, errKeyEmpty
panic(errKeyEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we panic over error? Ideally, the caller can handle the error.

Copy link
Collaborator Author

@yihuang yihuang Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just trying to make it similar to KVStore interface, it's usage in the store is side with the parent store, so i try to make the interface similar to the parent store.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I'd argue those interfaces are bad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, fixed.

}
return NewMemIterator(start, end, bt, make(map[string]struct{}), true), nil
return NewMemIterator(start, end, bt, true)
}

func (bt *BTree) ReverseIterator(start, end []byte) (*memIterator, error) { //nolint:revive
func (bt BTree) ReverseIterator(start, end []byte) types.Iterator {
if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
return nil, errKeyEmpty
panic(errKeyEmpty)
}
return NewMemIterator(start, end, bt, false)
}

// Copy the tree. This is a copy-on-write operation and is very fast because
// it only performs a shadowed copy.
func (bt BTree) Copy() BTree {
return BTree{
tree: bt.tree.Copy(),
}
return NewMemIterator(start, end, bt, make(map[string]struct{}), false), nil
}

// item is a btree item with byte slices as keys and values
Expand Down
102 changes: 38 additions & 64 deletions store/cachekv/internal/btree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package internal
import (
"testing"

"github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -48,140 +49,113 @@ func TestDBIterator(t *testing.T) {
}

// Blank iterator keys should error
_, err := db.ReverseIterator([]byte{}, nil)
require.Equal(t, errKeyEmpty, err)
_, err = db.ReverseIterator(nil, []byte{})
require.Equal(t, errKeyEmpty, err)

itr, err := db.Iterator(nil, nil)
require.NoError(t, err)
require.PanicsWithError(t, errKeyEmpty.Error(), func() {
db.ReverseIterator([]byte{}, nil)
})
require.PanicsWithError(t, errKeyEmpty.Error(), func() {
db.ReverseIterator(nil, []byte{})
})

itr := db.Iterator(nil, nil)
verifyIterator(t, itr, []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator")

ritr, err := db.ReverseIterator(nil, nil)
require.NoError(t, err)
ritr := db.ReverseIterator(nil, nil)
verifyIterator(t, ritr, []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator")

itr, err = db.Iterator(nil, int642Bytes(0))
require.NoError(t, err)
itr = db.Iterator(nil, int642Bytes(0))
verifyIterator(t, itr, []int64(nil), "forward iterator to 0")

ritr, err = db.ReverseIterator(int642Bytes(10), nil)
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(10), nil)
verifyIterator(t, ritr, []int64(nil), "reverse iterator from 10 (ex)")

itr, err = db.Iterator(int642Bytes(0), nil)
require.NoError(t, err)
itr = db.Iterator(int642Bytes(0), nil)
verifyIterator(t, itr, []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 0")

itr, err = db.Iterator(int642Bytes(1), nil)
require.NoError(t, err)
itr = db.Iterator(int642Bytes(1), nil)
verifyIterator(t, itr, []int64{1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 1")

ritr, err = db.ReverseIterator(nil, int642Bytes(10))
require.NoError(t, err)
ritr = db.ReverseIterator(nil, int642Bytes(10))
verifyIterator(t, ritr,
[]int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 10 (ex)")

ritr, err = db.ReverseIterator(nil, int642Bytes(9))
require.NoError(t, err)
ritr = db.ReverseIterator(nil, int642Bytes(9))
verifyIterator(t, ritr,
[]int64{8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 9 (ex)")

ritr, err = db.ReverseIterator(nil, int642Bytes(8))
require.NoError(t, err)
ritr = db.ReverseIterator(nil, int642Bytes(8))
verifyIterator(t, ritr,
[]int64{7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 8 (ex)")

itr, err = db.Iterator(int642Bytes(5), int642Bytes(6))
require.NoError(t, err)
itr = db.Iterator(int642Bytes(5), int642Bytes(6))
verifyIterator(t, itr, []int64{5}, "forward iterator from 5 to 6")

itr, err = db.Iterator(int642Bytes(5), int642Bytes(7))
require.NoError(t, err)
itr = db.Iterator(int642Bytes(5), int642Bytes(7))
verifyIterator(t, itr, []int64{5}, "forward iterator from 5 to 7")

itr, err = db.Iterator(int642Bytes(5), int642Bytes(8))
require.NoError(t, err)
itr = db.Iterator(int642Bytes(5), int642Bytes(8))
verifyIterator(t, itr, []int64{5, 7}, "forward iterator from 5 to 8")

itr, err = db.Iterator(int642Bytes(6), int642Bytes(7))
require.NoError(t, err)
itr = db.Iterator(int642Bytes(6), int642Bytes(7))
verifyIterator(t, itr, []int64(nil), "forward iterator from 6 to 7")

itr, err = db.Iterator(int642Bytes(6), int642Bytes(8))
require.NoError(t, err)
itr = db.Iterator(int642Bytes(6), int642Bytes(8))
verifyIterator(t, itr, []int64{7}, "forward iterator from 6 to 8")

itr, err = db.Iterator(int642Bytes(7), int642Bytes(8))
require.NoError(t, err)
itr = db.Iterator(int642Bytes(7), int642Bytes(8))
verifyIterator(t, itr, []int64{7}, "forward iterator from 7 to 8")

ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(5))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(5))
verifyIterator(t, ritr, []int64{4}, "reverse iterator from 5 (ex) to 4")

ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(6))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(6))
verifyIterator(t, ritr,
[]int64{5, 4}, "reverse iterator from 6 (ex) to 4")

ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(7))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(7))
verifyIterator(t, ritr,
[]int64{5, 4}, "reverse iterator from 7 (ex) to 4")

ritr, err = db.ReverseIterator(int642Bytes(5), int642Bytes(6))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(5), int642Bytes(6))
verifyIterator(t, ritr, []int64{5}, "reverse iterator from 6 (ex) to 5")

ritr, err = db.ReverseIterator(int642Bytes(5), int642Bytes(7))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(5), int642Bytes(7))
verifyIterator(t, ritr, []int64{5}, "reverse iterator from 7 (ex) to 5")

ritr, err = db.ReverseIterator(int642Bytes(6), int642Bytes(7))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(6), int642Bytes(7))
verifyIterator(t, ritr,
[]int64(nil), "reverse iterator from 7 (ex) to 6")

ritr, err = db.ReverseIterator(int642Bytes(10), nil)
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(10), nil)
verifyIterator(t, ritr, []int64(nil), "reverse iterator to 10")

ritr, err = db.ReverseIterator(int642Bytes(6), nil)
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(6), nil)
verifyIterator(t, ritr, []int64{9, 8, 7}, "reverse iterator to 6")

ritr, err = db.ReverseIterator(int642Bytes(5), nil)
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(5), nil)
verifyIterator(t, ritr, []int64{9, 8, 7, 5}, "reverse iterator to 5")

ritr, err = db.ReverseIterator(int642Bytes(8), int642Bytes(9))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(8), int642Bytes(9))
verifyIterator(t, ritr, []int64{8}, "reverse iterator from 9 (ex) to 8")

ritr, err = db.ReverseIterator(int642Bytes(2), int642Bytes(4))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(2), int642Bytes(4))
verifyIterator(t, ritr,
[]int64{3, 2}, "reverse iterator from 4 (ex) to 2")

ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(2))
require.NoError(t, err)
ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(2))
verifyIterator(t, ritr,
[]int64(nil), "reverse iterator from 2 (ex) to 4")

// Ensure that the iterators don't panic with an empty database.
db2 := NewBTree()

itr, err = db2.Iterator(nil, nil)
require.NoError(t, err)
itr = db2.Iterator(nil, nil)
verifyIterator(t, itr, nil, "forward iterator with empty db")

ritr, err = db2.ReverseIterator(nil, nil)
require.NoError(t, err)
ritr = db2.ReverseIterator(nil, nil)
verifyIterator(t, ritr, nil, "reverse iterator with empty db")
}

func verifyIterator(t *testing.T, itr *memIterator, expected []int64, msg string) {
func verifyIterator(t *testing.T, itr types.Iterator, expected []int64, msg string) {
i := 0
for itr.Valid() {
key := itr.Key()
Expand Down
24 changes: 3 additions & 21 deletions store/cachekv/internal/memiterator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,18 @@ import (
var _ types.Iterator = (*memIterator)(nil)

// memIterator iterates over iterKVCache items.
// if key is nil, means it was deleted.
// if value is nil, means it was deleted.
// Implements Iterator.
type memIterator struct {
iter btree.IterG[item]

start []byte
end []byte
ascending bool
lastKey []byte
deleted map[string]struct{}
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
valid bool
}

func NewMemIterator(start, end []byte, items *BTree, deleted map[string]struct{}, ascending bool) *memIterator { //nolint:revive
func NewMemIterator(start, end []byte, items BTree, ascending bool) *memIterator { //nolint:revive
iter := items.tree.Iter()
var valid bool
if ascending {
Expand Down Expand Up @@ -52,8 +50,6 @@ func NewMemIterator(start, end []byte, items *BTree, deleted map[string]struct{}
start: start,
end: end,
ascending: ascending,
lastKey: nil,
deleted: deleted,
valid: valid,
}

Expand Down Expand Up @@ -113,21 +109,7 @@ func (mi *memIterator) Key() []byte {
}

func (mi *memIterator) Value() []byte {
item := mi.iter.Item()
key := item.key
// We need to handle the case where deleted is modified and includes our current key
// We handle this by maintaining a lastKey object in the iterator.
// If the current key is the same as the last key (and last key is not nil / the start)
// then we are calling value on the same thing as last time.
// Therefore we don't check the mi.deleted to see if this key is included in there.
if _, ok := mi.deleted[string(key)]; ok {
if mi.lastKey == nil || !bytes.Equal(key, mi.lastKey) {
// not re-calling on old last key
return nil
}
}
mi.lastKey = key
return item.value
return mi.iter.Item().value
}

func (mi *memIterator) assertValid() {
Expand Down
Loading