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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Update prefixdb_test.go Co-authored-by: M. J. Fromberger <[email protected]> * Update prefixdb_test.go Co-authored-by: M. J. Fromberger <[email protected]> * Firing this into CI, let's see. * add filepath lib to tests * fix filepath in goleveldb * gofumpt formatting Co-authored-by: jess <[email protected]> Co-authored-by: vuong <[email protected]> Co-authored-by: khanh <[email protected]> Co-authored-by: M. J. Fromberger <[email protected]> Co-authored-by: romelukaku <[email protected]>
tendermint · Jul 29, 2022 · 00fb04a · 00fb04a
1 parent d1b9b74
commit 00fb04a
Show file tree

Hide file tree

Showing 21 changed files with 192 additions and 51 deletions.
diff --git a/.gitpod.yml 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1,3 +1,4 @@
+//go:build boltdb
 // +build boltdb
 
 package db

diff --git 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
@@ -1,10 +1,12 @@
+//go:build boltdb
 // +build boltdb
 
 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
@@ -1,3 +1,4 @@
+//go:build cleveldb
 // +build cleveldb
 
 package db

diff --git 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
@@ -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,36 +57,31 @@ func (itr cLevelDBIterator) Domain() ([]byte, []byte) {
 
 // Valid implements Iterator.
 func (itr cLevelDBIterator) Valid() bool {
-
 	// Once invalid, forever invalid.
 	if itr.isInvalid {
 		return false
 	}
 
 	// 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
@@ -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
@@ -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
@@ -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
@@ -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.