From 00fb04a8265a4bf7464b20fab7e320995d37631c Mon Sep 17 00:00:00 2001 From: Jacob Gadikian Date: Fri, 29 Jul 2022 14:49:07 +0700 Subject: [PATCH] fix: remove mutex in prefixdb (#239) * fix: remove mutex in prefixdb * fmt * fmt * Delete .gitpod.yml * remove mutex entirely instead of commenting out * fmt * remove .gitpod.yml * update changelog * prefixdb test * all test passed in prefixdb_test.go * simple taskKey in prefixdb_test.go * Update prefixdb.go Co-authored-by: M. J. Fromberger * fmt db test so pr passes linter * refactor func Run * check nil err before create PrefixDB in TestWithGolevelDB * add other db tests * Update prefixdb_test.go Co-authored-by: M. J. Fromberger * update prefixdb_test.go * minor defer * fixup with build tags * Create .gitpod.yml * fix linting * itr.isInvalid would be unconditionally set anyhow, which is why the itr.isInvalid were considered to be ineffective assignments by the linter. * Update cleveldb_test.go Co-authored-by: M. J. Fromberger * Update prefixdb_test.go Co-authored-by: M. J. Fromberger * Update prefixdb_test.go Co-authored-by: M. J. Fromberger * Firing this into CI, let's see. * add filepath lib to tests * fix filepath in goleveldb * gofumpt formatting Co-authored-by: jess Co-authored-by: vuong Co-authored-by: khanh <50263489+catShaark@users.noreply.github.com> Co-authored-by: M. J. Fromberger Co-authored-by: romelukaku --- .gitpod.yml | 7 +++ .golangci.yml | 7 +++ CHANGELOG.md | 4 ++ backend_test.go | 1 - badger_db.go | 2 +- boltdb.go | 10 ++-- boltdb_batch.go | 1 + boltdb_iterator.go | 1 + boltdb_test.go | 12 +++++ cleveldb.go | 1 + cleveldb_batch.go | 1 + cleveldb_iterator.go | 16 +++---- cleveldb_test.go | 22 +++++++-- goleveldb_iterator.go | 7 ++- prefixdb.go | 19 -------- prefixdb_iterator.go | 1 - prefixdb_test.go | 109 ++++++++++++++++++++++++++++++++++++++++++ rocksdb.go | 1 + rocksdb_batch.go | 1 + rocksdb_iterator.go | 8 ++-- rocksdb_test.go | 12 +++++ 21 files changed, 192 insertions(+), 51 deletions(-) create mode 100644 .gitpod.yml diff --git a/.gitpod.yml b/.gitpod.yml new file mode 100644 index 000000000..4b56b6b70 --- /dev/null +++ b/.gitpod.yml @@ -0,0 +1,7 @@ +# This configuration file was automatically generated by Gitpod. +# Please adjust to your needs (see https://www.gitpod.io/docs/config-gitpod-file) +# and commit this file to your remote git repository to share the goodness with others. + +image: tendermintdev/docker-tm-db-testing + +# this means that there's a one-click known good environment available to developers. diff --git a/.golangci.yml b/.golangci.yml index abf03e7ac..4fd7732da 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -40,6 +40,13 @@ linters: # - gocognit - nolintlint +run: + build-tags: + - cleveldb + - rocksdb + - badgerdb + - boltdb + issues: exclude-rules: - path: _test\.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 607f0bafb..9654735be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- remove mutex from prefixdb + ## 0.6.7 **2022-2-21** diff --git a/backend_test.go b/backend_test.go index 63630beca..aecae4bf5 100644 --- a/backend_test.go +++ b/backend_test.go @@ -315,7 +315,6 @@ func testDBIterator(t *testing.T, backend BackendType) { ritr, err = db2.ReverseIterator(nil, nil) require.NoError(t, err) verifyIterator(t, ritr, nil, "reverse iterator with empty db") - } func verifyIterator(t *testing.T, itr Iterator, expected []int64, msg string) { diff --git a/badger_db.go b/badger_db.go index 193372abf..467ed3361 100644 --- a/badger_db.go +++ b/badger_db.go @@ -25,7 +25,7 @@ func NewBadgerDB(dbName, dir string) (*BadgerDB, error) { // the final directory to use for the database. path := filepath.Join(dir, dbName) - if err := os.MkdirAll(path, 0755); err != nil { + if err := os.MkdirAll(path, 0o755); err != nil { return nil, err } opts := badger.DefaultOptions(path) diff --git a/boltdb.go b/boltdb.go index 398c401a4..b1f69983d 100644 --- a/boltdb.go +++ b/boltdb.go @@ -1,3 +1,4 @@ +//go:build boltdb // +build boltdb package db @@ -11,14 +12,10 @@ import ( "go.etcd.io/bbolt" ) -var ( - bucket = []byte("tm") -) +var bucket = []byte("tm") func init() { - registerDBCreator(BoltDBBackend, func(name, dir string) (DB, error) { - return NewBoltDB(name, dir) - }, false) + registerDBCreator(BoltDBBackend, NewBoltDB, false) } // BoltDB is a wrapper around etcd's fork of bolt (https://github.com/etcd-io/bbolt). @@ -139,6 +136,7 @@ func (bdb *BoltDB) Close() error { } // Print implements DB. +// nolint: errcheck func (bdb *BoltDB) Print() error { stats := bdb.db.Stats() fmt.Printf("%v\n", stats) diff --git a/boltdb_batch.go b/boltdb_batch.go index 390bc309b..cd22c6741 100644 --- a/boltdb_batch.go +++ b/boltdb_batch.go @@ -1,3 +1,4 @@ +//go:build boltdb // +build boltdb package db diff --git a/boltdb_iterator.go b/boltdb_iterator.go index 74212c5ca..a62e2abb8 100644 --- a/boltdb_iterator.go +++ b/boltdb_iterator.go @@ -1,3 +1,4 @@ +//go:build boltdb // +build boltdb package db diff --git a/boltdb_test.go b/boltdb_test.go index 0fd12bbe2..02a9eaf44 100644 --- a/boltdb_test.go +++ b/boltdb_test.go @@ -1,3 +1,4 @@ +//go:build boltdb // +build boltdb package db @@ -5,6 +6,7 @@ package db import ( "fmt" "os" + "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -20,6 +22,16 @@ func TestBoltDBNewBoltDB(t *testing.T) { db.Close() } +func TestWithBoltDB(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "boltdb") + + db, err := NewBoltDB(path, "") + require.NoError(t, err) + + t.Run("BoltDB", func(t *testing.T) { Run(t, db) }) +} + func BenchmarkBoltDBRandomReadsWrites(b *testing.B) { name := fmt.Sprintf("test_%x", randStr(12)) db, err := NewBoltDB(name, "") diff --git a/cleveldb.go b/cleveldb.go index 377956deb..789673011 100644 --- a/cleveldb.go +++ b/cleveldb.go @@ -1,3 +1,4 @@ +//go:build cleveldb // +build cleveldb package db diff --git a/cleveldb_batch.go b/cleveldb_batch.go index 132a521cb..b77bd523d 100644 --- a/cleveldb_batch.go +++ b/cleveldb_batch.go @@ -1,3 +1,4 @@ +//go:build cleveldb // +build cleveldb package db diff --git a/cleveldb_iterator.go b/cleveldb_iterator.go index 04375d565..5a7f1e6ad 100644 --- a/cleveldb_iterator.go +++ b/cleveldb_iterator.go @@ -1,3 +1,4 @@ +//go:build cleveldb // +build cleveldb package db @@ -20,7 +21,7 @@ var _ Iterator = (*cLevelDBIterator)(nil) func newCLevelDBIterator(source *levigo.Iterator, start, end []byte, isReverse bool) *cLevelDBIterator { if isReverse { - if end == nil || len(end) == 0 { + if len(end) == 0 { source.SeekToLast() } else { source.Seek(end) @@ -34,7 +35,7 @@ func newCLevelDBIterator(source *levigo.Iterator, start, end []byte, isReverse b } } } else { - if start == nil || len(start) == 0 { + if len(start) == 0 { source.SeekToFirst() } else { source.Seek(start) @@ -56,7 +57,6 @@ func (itr cLevelDBIterator) Domain() ([]byte, []byte) { // Valid implements Iterator. func (itr cLevelDBIterator) Valid() bool { - // Once invalid, forever invalid. if itr.isInvalid { return false @@ -64,28 +64,24 @@ func (itr cLevelDBIterator) Valid() bool { // If source errors, invalid. if itr.source.GetError() != nil { - itr.isInvalid = true return false } // If source is invalid, invalid. if !itr.source.Valid() { - itr.isInvalid = true return false } // If key is end or past it, invalid. - var start = itr.start - var end = itr.end - var key = itr.source.Key() + start := itr.start + end := itr.end + key := itr.source.Key() if itr.isReverse { if start != nil && bytes.Compare(key, start) < 0 { - itr.isInvalid = true return false } } else { if end != nil && bytes.Compare(end, key) <= 0 { - itr.isInvalid = true return false } } diff --git a/cleveldb_test.go b/cleveldb_test.go index 61e2fb6ef..55fca6b9f 100644 --- a/cleveldb_test.go +++ b/cleveldb_test.go @@ -1,3 +1,4 @@ +//go:build cleveldb // +build cleveldb package db @@ -7,12 +8,24 @@ import ( "fmt" "math/rand" "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestWithClevelDB(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "cleveldb") + + db, err := NewCLevelDB(path, "") + require.NoError(t, err) + + t.Run("ClevelDB", func(t *testing.T) { Run(t, db) }) +} + +//nolint: errcheck func BenchmarkRandomReadsWrites2(b *testing.B) { b.StopTimer() @@ -36,9 +49,8 @@ func BenchmarkRandomReadsWrites2(b *testing.B) { idx := (int64(rand.Int()) % numItems) internal[idx]++ val := internal[idx] - idxBytes := int642Bytes(int64(idx)) - valBytes := int642Bytes(int64(val)) - //fmt.Printf("Set %X -> %X\n", idxBytes, valBytes) + idxBytes := int642Bytes(idx) + valBytes := int642Bytes(val) db.Set( idxBytes, valBytes, @@ -48,12 +60,12 @@ func BenchmarkRandomReadsWrites2(b *testing.B) { { idx := (int64(rand.Int()) % numItems) val := internal[idx] - idxBytes := int642Bytes(int64(idx)) + idxBytes := int642Bytes(idx) valBytes, err := db.Get(idxBytes) if err != nil { b.Error(err) } - //fmt.Printf("Get %X -> %X\n", idxBytes, valBytes) + // fmt.Printf("Get %X -> %X\n", idxBytes, valBytes) if val == 0 { if !bytes.Equal(valBytes, nil) { b.Errorf("Expected %v for %v, got %X", diff --git a/goleveldb_iterator.go b/goleveldb_iterator.go index 9cf9a5e15..5341d1ac2 100644 --- a/goleveldb_iterator.go +++ b/goleveldb_iterator.go @@ -54,7 +54,6 @@ func (itr *goLevelDBIterator) Domain() ([]byte, []byte) { // Valid implements Iterator. func (itr *goLevelDBIterator) Valid() bool { - // Once invalid, forever invalid. if itr.isInvalid { return false @@ -73,9 +72,9 @@ func (itr *goLevelDBIterator) Valid() bool { } // If key is end or past it, invalid. - var start = itr.start - var end = itr.end - var key = itr.source.Key() + start := itr.start + end := itr.end + key := itr.source.Key() if itr.isReverse { if start != nil && bytes.Compare(key, start) < 0 { diff --git a/prefixdb.go b/prefixdb.go index 0b2d2a1cf..447ce2e9b 100644 --- a/prefixdb.go +++ b/prefixdb.go @@ -27,8 +27,6 @@ func (pdb *PrefixDB) Get(key []byte) ([]byte, error) { if len(key) == 0 { return nil, errKeyEmpty } - pdb.mtx.Lock() - defer pdb.mtx.Unlock() pkey := pdb.prefixed(key) value, err := pdb.db.Get(pkey) @@ -43,8 +41,6 @@ func (pdb *PrefixDB) Has(key []byte) (bool, error) { if len(key) == 0 { return false, errKeyEmpty } - pdb.mtx.Lock() - defer pdb.mtx.Unlock() ok, err := pdb.db.Has(pdb.prefixed(key)) if err != nil { @@ -62,8 +58,6 @@ func (pdb *PrefixDB) Set(key []byte, value []byte) error { if value == nil { return errValueNil } - pdb.mtx.Lock() - defer pdb.mtx.Unlock() pkey := pdb.prefixed(key) if err := pdb.db.Set(pkey, value); err != nil { @@ -80,8 +74,6 @@ func (pdb *PrefixDB) SetSync(key []byte, value []byte) error { if value == nil { return errValueNil } - pdb.mtx.Lock() - defer pdb.mtx.Unlock() return pdb.db.SetSync(pdb.prefixed(key), value) } @@ -91,8 +83,6 @@ func (pdb *PrefixDB) Delete(key []byte) error { if len(key) == 0 { return errKeyEmpty } - pdb.mtx.Lock() - defer pdb.mtx.Unlock() return pdb.db.Delete(pdb.prefixed(key)) } @@ -102,8 +92,6 @@ func (pdb *PrefixDB) DeleteSync(key []byte) error { if len(key) == 0 { return errKeyEmpty } - pdb.mtx.Lock() - defer pdb.mtx.Unlock() return pdb.db.DeleteSync(pdb.prefixed(key)) } @@ -113,8 +101,6 @@ func (pdb *PrefixDB) Iterator(start, end []byte) (Iterator, error) { if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) { return nil, errKeyEmpty } - pdb.mtx.Lock() - defer pdb.mtx.Unlock() var pstart, pend []byte pstart = append(cp(pdb.prefix), start...) @@ -136,8 +122,6 @@ func (pdb *PrefixDB) ReverseIterator(start, end []byte) (Iterator, error) { if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) { return nil, errKeyEmpty } - pdb.mtx.Lock() - defer pdb.mtx.Unlock() var pstart, pend []byte pstart = append(cp(pdb.prefix), start...) @@ -156,9 +140,6 @@ func (pdb *PrefixDB) ReverseIterator(start, end []byte) (Iterator, error) { // NewBatch implements DB. func (pdb *PrefixDB) NewBatch() Batch { - pdb.mtx.Lock() - defer pdb.mtx.Unlock() - return newPrefixBatch(pdb.prefix, pdb.db.NewBatch()) } diff --git a/prefixdb_iterator.go b/prefixdb_iterator.go index 79e1ef7fa..8f2f93262 100644 --- a/prefixdb_iterator.go +++ b/prefixdb_iterator.go @@ -91,7 +91,6 @@ func (itr *prefixDBIterator) Next() { if !itr.source.Valid() || !bytes.HasPrefix(itr.source.Key(), itr.prefix) { itr.valid = false - } else if bytes.Equal(itr.source.Key(), itr.prefix) { // Empty keys are not allowed, so if a key exists in the database that exactly matches the // prefix we need to skip it. diff --git a/prefixdb_test.go b/prefixdb_test.go index 3fc53ee92..07db733df 100644 --- a/prefixdb_test.go +++ b/prefixdb_test.go @@ -1,6 +1,12 @@ package db import ( + "bytes" + "encoding/binary" + "fmt" + "math/rand" + "path/filepath" + "sync" "testing" "github.com/stretchr/testify/require" @@ -20,6 +26,109 @@ func mockDBWithStuff(t *testing.T) DB { return db } +func taskKey(i, k int) []byte { + return []byte(fmt.Sprintf("task-%d-key-%d", i, k)) +} + +func randomValue() []byte { + b := make([]byte, 16) + rand.Read(b) + return b +} + +func TestGolevelDB(t *testing.T) { + path := filepath.Join(t.TempDir(), "goleveldb") + + db, err := NewGoLevelDB(path, "") + require.NoError(t, err) + + Run(t, db) +} + +/* We don't seem to test badger anywhere. +func TestWithBadgerDB(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "badgerdb") + + db, err := NewBadgerDB(path, "") + require.NoError(t, err) + + t.Run("BadgerDB", func(t *testing.T) { Run(t, db) }) +} +*/ + +func TestWithMemDB(t *testing.T) { + db := NewMemDB() + + t.Run("MemDB", func(t *testing.T) { Run(t, db) }) +} + +// Run generates concurrent reads and writes to db so the race detector can +// verify concurrent operations are properly synchronized. +// The contents of db are garbage after Run returns. +func Run(t *testing.T, db DB) { + t.Helper() + + const numWorkers = 10 + const numKeys = 64 + + var wg sync.WaitGroup + for i := 0; i < numWorkers; i++ { + wg.Add(1) + i := i + go func() { + defer wg.Done() + + // Insert a bunch of keys with random data. + for k := 1; k <= numKeys; k++ { + key := taskKey(i, k) // say, "task--key-" + value := randomValue() + if err := db.Set(key, value); err != nil { + t.Errorf("Task %d: db.Set(%q=%q) failed: %v", + i, string(key), string(value), err) + } + } + + // Iterate over the database to make sure our keys are there. + it, err := db.Iterator(nil, nil) + if err != nil { + t.Errorf("Iterator[%d]: %v", i, err) + return + } + found := make(map[string][]byte) + mine := []byte(fmt.Sprintf("task-%d-", i)) + for { + if key := it.Key(); bytes.HasPrefix(key, mine) { + found[string(key)] = it.Value() + } + it.Next() + if !it.Valid() { + break + } + } + if err := it.Error(); err != nil { + t.Errorf("Iterator[%d] reported error: %v", i, err) + } + if err := it.Close(); err != nil { + t.Errorf("Close iterator[%d]: %v", i, err) + } + if len(found) != numKeys { + t.Errorf("Task %d: found %d keys, wanted %d", i, len(found), numKeys) + } + + // Delete all the keys we inserted. + for key := range mine { + bs := make([]byte, 4) + binary.LittleEndian.PutUint32(bs, uint32(key)) + if err := db.Delete(bs); err != nil { + t.Errorf("Delete %q: %v", key, err) + } + } + }() + } + wg.Wait() +} + func TestPrefixDBSimple(t *testing.T) { db := mockDBWithStuff(t) pdb := NewPrefixDB(db, bz("key")) diff --git a/rocksdb.go b/rocksdb.go index dd9be2a36..06d056dd8 100644 --- a/rocksdb.go +++ b/rocksdb.go @@ -1,3 +1,4 @@ +//go:build rocksdb // +build rocksdb package db diff --git a/rocksdb_batch.go b/rocksdb_batch.go index af7e65c60..b2557ac45 100644 --- a/rocksdb_batch.go +++ b/rocksdb_batch.go @@ -1,3 +1,4 @@ +//go:build rocksdb // +build rocksdb package db diff --git a/rocksdb_iterator.go b/rocksdb_iterator.go index 9970c169e..e79a76cd6 100644 --- a/rocksdb_iterator.go +++ b/rocksdb_iterator.go @@ -1,3 +1,4 @@ +//go:build rocksdb // +build rocksdb package db @@ -55,7 +56,6 @@ func (itr *rocksDBIterator) Domain() ([]byte, []byte) { // Valid implements Iterator. func (itr *rocksDBIterator) Valid() bool { - // Once invalid, forever invalid. if itr.isInvalid { return false @@ -74,9 +74,9 @@ func (itr *rocksDBIterator) Valid() bool { } // If key is end or past it, invalid. - var start = itr.start - var end = itr.end - var key = moveSliceToBytes(itr.source.Key()) + start := itr.start + end := itr.end + key := moveSliceToBytes(itr.source.Key()) if itr.isReverse { if start != nil && bytes.Compare(key, start) < 0 { itr.isInvalid = true diff --git a/rocksdb_test.go b/rocksdb_test.go index 6bbe51133..bd6793420 100644 --- a/rocksdb_test.go +++ b/rocksdb_test.go @@ -1,3 +1,4 @@ +//go:build rocksdb // +build rocksdb package db @@ -5,6 +6,7 @@ package db import ( "fmt" "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -22,6 +24,16 @@ func TestRocksDBBackend(t *testing.T) { assert.True(t, ok) } +func TestWithRocksDB(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "rocksdb") + + db, err := NewRocksDB(path, "") + require.NoError(t, err) + + t.Run("RocksDB", func(t *testing.T) { Run(t, db) }) +} + func TestRocksDBStats(t *testing.T) { name := fmt.Sprintf("test_%x", randStr(12)) dir := os.TempDir()