From f191247345205a7428b77dda8770188e024a564e Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Mon, 18 May 2020 13:57:50 +0200 Subject: [PATCH] improve error handling (#98) * remove pkg/errors * don't panic in RemoteDB.Stats() * fix some panics * update tests * changelog tweaks --- CHANGELOG.md | 15 +++++++++------ backend_test.go | 19 ++++++++++++------- boltdb.go | 2 +- cleveldb_test.go | 7 +++++-- common_test.go | 6 ++++-- db.go | 18 +++++++----------- go.mod | 1 - go.sum | 6 ------ memdb_batch.go | 4 ++-- remotedb/batch.go | 7 ++++--- remotedb/grpcdb/server.go | 6 +++++- remotedb/remotedb.go | 18 +++++++----------- rocksdb_test.go | 7 +++++-- 13 files changed, 61 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39ea18b1f..05646b443 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,16 +4,19 @@ ### Breaking Changes -- Batch `Set()`, `Delete()`, and `Close()` may now error (@erikgrinaker) +- [\#98](https://github.com/tendermint/tm-db/pull/98) `NewDB` now returns an error instead of panicing (@erikgrinaker) -- `Iterator.Close()` may now error (@erikgrinaker) +- [\#96](https://github.com/tendermint/tm-db/pull/96) `Batch.Set()`, `Delete()`, and `Close()` may now error (@erikgrinaker) -- Many iterator panics are now exposed via `Error()` instead (@erikgrinaker) +- [\#97](https://github.com/tendermint/tm-db/pull/97) `Iterator.Close()` may now error (@erikgrinaker) -- `RemoteDB` iterators are now correctly primed with the first item when created, without calling - `Next()` (@erikgrinaker) +- [\#97](https://github.com/tendermint/tm-db/pull/97) Many iterator panics are now exposed via `Error()` instead (@erikgrinaker) -- The `SetDeleter` interface has been removed (@erikgrinaker) +- [\#96](https://github.com/tendermint/tm-db/pull/96) The `SetDeleter` interface has been removed (@erikgrinaker) + +### Bug Fixes + +- [\#97](https://github.com/tendermint/tm-db/pull/97) `RemoteDB` iterators are now correctly primed with the first item when created, without calling `Next()` (@erikgrinaker) ## 0.5.1 diff --git a/backend_test.go b/backend_test.go index 1b0e14200..7224c8374 100644 --- a/backend_test.go +++ b/backend_test.go @@ -37,7 +37,8 @@ func testBackendGetSetDelete(t *testing.T, backend BackendType) { // Default dirname, err := ioutil.TempDir("", fmt.Sprintf("test_backend_%s_", backend)) require.Nil(t, err) - db := NewDB("testdb", backend, dirname) + db, err := NewDB("testdb", backend, dirname) + require.NoError(t, err) defer cleanupDBDir(dirname, "testdb") // A nonexistent key should return nil, even if the key is empty @@ -212,7 +213,8 @@ func TestBackendsNilKeys(t *testing.T) { func TestGoLevelDBBackend(t *testing.T) { name := fmt.Sprintf("test_%x", randStr(12)) - db := NewDB(name, GoLevelDBBackend, "") + db, err := NewDB(name, GoLevelDBBackend, "") + require.NoError(t, err) defer cleanupDBDir("", name) _, ok := db.(*GoLevelDB) @@ -231,7 +233,8 @@ func TestDBIterator(t *testing.T) { func testDBIterator(t *testing.T, backend BackendType) { name := fmt.Sprintf("test_%x", randStr(12)) dir := os.TempDir() - db := NewDB(name, backend, dir) + db, err := NewDB(name, backend, dir) + require.NoError(t, err) defer cleanupDBDir(dir, name) for i := 0; i < 10; i++ { @@ -366,10 +369,11 @@ func testDBIterator(t *testing.T, backend BackendType) { func testDBIteratorBlankKey(t *testing.T, backend BackendType) { name := fmt.Sprintf("test_%x", randStr(12)) dir := os.TempDir() - db := NewDB(name, backend, dir) + db, err := NewDB(name, backend, dir) + require.NoError(t, err) defer cleanupDBDir(dir, name) - err := db.Set([]byte(""), []byte{0}) + err = db.Set([]byte(""), []byte{0}) require.NoError(t, err) err = db.Set([]byte("a"), []byte{1}) require.NoError(t, err) @@ -444,7 +448,8 @@ func TestDBBatch(t *testing.T) { func testDBBatch(t *testing.T, backend BackendType) { name := fmt.Sprintf("test_%x", randStr(12)) dir := os.TempDir() - db := NewDB(name, backend, dir) + db, err := NewDB(name, backend, dir) + require.NoError(t, err) defer cleanupDBDir(dir, name) // create a new batch, and some items - they should not be visible until we write @@ -454,7 +459,7 @@ func testDBBatch(t *testing.T, backend BackendType) { require.NoError(t, batch.Set([]byte("c"), []byte{3})) assertKeyValues(t, db, map[string][]byte{}) - err := batch.Write() + err = batch.Write() require.NoError(t, err) assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}, "c": {3}}) diff --git a/boltdb.go b/boltdb.go index 4fa89fb8f..b589f05f6 100644 --- a/boltdb.go +++ b/boltdb.go @@ -3,11 +3,11 @@ package db import ( + "errors" "fmt" "os" "path/filepath" - "github.com/pkg/errors" "go.etcd.io/bbolt" ) diff --git a/cleveldb_test.go b/cleveldb_test.go index e893c09af..61e2fb6ef 100644 --- a/cleveldb_test.go +++ b/cleveldb_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func BenchmarkRandomReadsWrites2(b *testing.B) { @@ -83,7 +84,8 @@ func TestCLevelDBBackend(t *testing.T) { // Can't use "" (current directory) or "./" here because levigo.Open returns: // "Error initializing DB: IO error: test_XXX.db: Invalid argument" dir := os.TempDir() - db := NewDB(name, CLevelDBBackend, dir) + db, err := NewDB(name, CLevelDBBackend, dir) + require.NoError(t, err) defer cleanupDBDir(dir, name) _, ok := db.(*CLevelDB) @@ -93,7 +95,8 @@ func TestCLevelDBBackend(t *testing.T) { func TestCLevelDBStats(t *testing.T) { name := fmt.Sprintf("test_%x", randStr(12)) dir := os.TempDir() - db := NewDB(name, CLevelDBBackend, dir) + db, err := NewDB(name, CLevelDBBackend, dir) + require.NoError(t, err) defer cleanupDBDir(dir, name) assert.NotEmpty(t, db.Stats()) diff --git a/common_test.go b/common_test.go index 766b3cbfa..f48efd7aa 100644 --- a/common_test.go +++ b/common_test.go @@ -68,8 +68,10 @@ func checkValuePanics(t *testing.T, itr Iterator) { func newTempDB(t *testing.T, backend BackendType) (db DB, dbDir string) { dirname, err := ioutil.TempDir("", "db_common_test") - require.Nil(t, err) - return NewDB("testdb", backend, dirname), dirname + require.NoError(t, err) + db, err = NewDB("testdb", backend, dirname) + require.NoError(t, err) + return db, dirname } func benchmarkRangeScans(b *testing.B, db DB, dbSize int64) { diff --git a/db.go b/db.go index 8972415bc..bc8b4a08d 100644 --- a/db.go +++ b/db.go @@ -48,24 +48,20 @@ func registerDBCreator(backend BackendType, creator dbCreator, force bool) { } // NewDB creates a new database of type backend with the given name. -// NOTE: function panics if: -// - backend is unknown (not registered) -// - creator function, provided during registration, returns error -func NewDB(name string, backend BackendType, dir string) DB { +func NewDB(name string, backend BackendType, dir string) (DB, error) { dbCreator, ok := backends[backend] if !ok { - keys := make([]string, len(backends)) - i := 0 + keys := make([]string, 0, len(backends)) for k := range backends { - keys[i] = string(k) - i++ + keys = append(keys, string(k)) } - panic(fmt.Sprintf("Unknown db_backend %s, expected either %s", backend, strings.Join(keys, " or "))) + return nil, fmt.Errorf("unknown db_backend %s, expected one of %v", + backend, strings.Join(keys, ",")) } db, err := dbCreator(name, dir) if err != nil { - panic(fmt.Sprintf("Error initializing DB: %v", err)) + return nil, fmt.Errorf("failed to initialize database: %w", err) } - return db + return db, nil } diff --git a/go.mod b/go.mod index bc874a841..bfc755b05 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,6 @@ require ( github.com/gogo/protobuf v1.3.1 github.com/google/btree v1.0.0 github.com/jmhodges/levigo v1.0.0 - github.com/pkg/errors v0.9.1 github.com/stretchr/testify v1.5.1 github.com/syndtr/goleveldb v1.0.1-0.20190923125748-758128399b1d github.com/tecbot/gorocksdb v0.0.0-20191217155057-f0fad39f321c diff --git a/go.sum b/go.sum index 0d0795327..43abc6c7f 100644 --- a/go.sum +++ b/go.sum @@ -45,8 +45,6 @@ github.com/onsi/ginkgo v1.7.0 h1:WSHQ+IS43OoUrWtD1/bbclrwK8TTH5hzp+umCiuxHgs= github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/gomega v1.4.3 h1:RE1xgDvH7imwFD45h+u2SgIfERHlS2yNG4DObb5BSKU= github.com/onsi/gomega v1.4.3/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= -github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= -github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= @@ -100,10 +98,6 @@ google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55/go.mod h1:DMBHOl98 google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c= google.golang.org/grpc v1.23.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg= google.golang.org/grpc v1.25.1/go.mod h1:c3i+UQWmh7LiEpx4sFZnkU36qjEYZ0imhYfXVyQciAY= -google.golang.org/grpc v1.28.1 h1:C1QC6KzgSiLyBabDi87BbjaGreoRgGUF5nOyvfrAZ1k= -google.golang.org/grpc v1.28.1/go.mod h1:rpkK4SK4GF4Ach/+MFLZUBavHOvF2JJB5uozKKal+60= -google.golang.org/grpc v1.29.0 h1:2pJjwYOdkZ9HlN4sWRYBg9ttH5bCOlsueaM+b/oYjwo= -google.golang.org/grpc v1.29.0/go.mod h1:itym6AZVZYACWQqET3MqgPpjcuV5QH3BxFS3IjizoKk= google.golang.org/grpc v1.29.1 h1:EC2SB8S04d2r73uptxphDSUG+kTKVgjRPF+N3xpxRB4= google.golang.org/grpc v1.29.1/go.mod h1:itym6AZVZYACWQqET3MqgPpjcuV5QH3BxFS3IjizoKk= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= diff --git a/memdb_batch.go b/memdb_batch.go index f308ed20e..4c412ee11 100644 --- a/memdb_batch.go +++ b/memdb_batch.go @@ -1,6 +1,6 @@ package db -import "github.com/pkg/errors" +import "fmt" // memDBBatch operations type opType int @@ -65,7 +65,7 @@ func (b *memDBBatch) Write() error { case opTypeDelete: b.db.delete(op.key) default: - return errors.Errorf("unknown operation type %v (%v)", op.opType, op) + return fmt.Errorf("unknown operation type %v (%v)", op.opType, op) } } diff --git a/remotedb/batch.go b/remotedb/batch.go index 68b5f17bd..fe2becb14 100644 --- a/remotedb/batch.go +++ b/remotedb/batch.go @@ -1,7 +1,8 @@ package remotedb import ( - "github.com/pkg/errors" + "errors" + "fmt" db "github.com/tendermint/tm-db" protodb "github.com/tendermint/tm-db/remotedb/proto" @@ -56,7 +57,7 @@ func (b *batch) Write() error { } _, err := b.db.dc.BatchWrite(b.db.ctx, &protodb.Batch{Ops: b.ops}) if err != nil { - return errors.Errorf("remoteDB.BatchWrite: %v", err) + return fmt.Errorf("remoteDB.BatchWrite: %w", err) } // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. b.Close() @@ -70,7 +71,7 @@ func (b *batch) WriteSync() error { } _, err := b.db.dc.BatchWriteSync(b.db.ctx, &protodb.Batch{Ops: b.ops}) if err != nil { - return errors.Errorf("RemoteDB.BatchWriteSync: %v", err) + return fmt.Errorf("RemoteDB.BatchWriteSync: %w", err) } // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. return b.Close() diff --git a/remotedb/grpcdb/server.go b/remotedb/grpcdb/server.go index 79dc78646..3cedca8c7 100644 --- a/remotedb/grpcdb/server.go +++ b/remotedb/grpcdb/server.go @@ -63,7 +63,11 @@ func (s *server) Init(ctx context.Context, in *protodb.Init) (*protodb.Entity, e s.mu.Lock() defer s.mu.Unlock() - s.db = db.NewDB(in.Name, db.BackendType(in.Type), in.Dir) + var err error + s.db, err = db.NewDB(in.Name, db.BackendType(in.Type), in.Dir) + if err != nil { + return nil, err + } return &protodb.Entity{CreatedAt: time.Now().Unix()}, nil } diff --git a/remotedb/remotedb.go b/remotedb/remotedb.go index 02822807c..84a57f20f 100644 --- a/remotedb/remotedb.go +++ b/remotedb/remotedb.go @@ -2,10 +2,9 @@ package remotedb import ( "context" + "errors" "fmt" - "github.com/pkg/errors" - db "github.com/tendermint/tm-db" "github.com/tendermint/tm-db/remotedb/grpcdb" protodb "github.com/tendermint/tm-db/remotedb/proto" @@ -47,28 +46,28 @@ func (rd *RemoteDB) Close() error { func (rd *RemoteDB) Delete(key []byte) error { if _, err := rd.dc.Delete(rd.ctx, &protodb.Entity{Key: key}); err != nil { - return errors.Errorf("remoteDB.Delete: %v", err) + return fmt.Errorf("remoteDB.Delete: %w", err) } return nil } func (rd *RemoteDB) DeleteSync(key []byte) error { if _, err := rd.dc.DeleteSync(rd.ctx, &protodb.Entity{Key: key}); err != nil { - return errors.Errorf("remoteDB.DeleteSync: %v", err) + return fmt.Errorf("remoteDB.DeleteSync: %w", err) } return nil } func (rd *RemoteDB) Set(key, value []byte) error { if _, err := rd.dc.Set(rd.ctx, &protodb.Entity{Key: key, Value: value}); err != nil { - return errors.Errorf("remoteDB.Set: %v", err) + return fmt.Errorf("remoteDB.Set: %w", err) } return nil } func (rd *RemoteDB) SetSync(key, value []byte) error { if _, err := rd.dc.SetSync(rd.ctx, &protodb.Entity{Key: key, Value: value}); err != nil { - return errors.Errorf("remoteDB.SetSync: %v", err) + return fmt.Errorf("remoteDB.SetSync: %w", err) } return nil } @@ -76,7 +75,7 @@ func (rd *RemoteDB) SetSync(key, value []byte) error { func (rd *RemoteDB) Get(key []byte) ([]byte, error) { res, err := rd.dc.Get(rd.ctx, &protodb.Entity{Key: key}) if err != nil { - return nil, errors.Errorf("remoteDB.Get error: %v", err) + return nil, fmt.Errorf("remoteDB.Get error: %w", err) } return res.Value, nil } @@ -109,10 +108,7 @@ func (rd *RemoteDB) Print() error { func (rd *RemoteDB) Stats() map[string]string { stats, err := rd.dc.Stats(rd.ctx, &protodb.Nothing{}) - if err != nil { - panic(fmt.Sprintf("RemoteDB.Stats error: %v", err)) - } - if stats == nil { + if err != nil || stats == nil { return nil } return stats.Data diff --git a/rocksdb_test.go b/rocksdb_test.go index dcd1fca91..6bbe51133 100644 --- a/rocksdb_test.go +++ b/rocksdb_test.go @@ -8,12 +8,14 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRocksDBBackend(t *testing.T) { name := fmt.Sprintf("test_%x", randStr(12)) dir := os.TempDir() - db := NewDB(name, RocksDBBackend, dir) + db, err := NewDB(name, RocksDBBackend, dir) + require.NoError(t, err) defer cleanupDBDir(dir, name) _, ok := db.(*RocksDB) @@ -23,7 +25,8 @@ func TestRocksDBBackend(t *testing.T) { func TestRocksDBStats(t *testing.T) { name := fmt.Sprintf("test_%x", randStr(12)) dir := os.TempDir() - db := NewDB(name, RocksDBBackend, dir) + db, err := NewDB(name, RocksDBBackend, dir) + require.NoError(t, err) defer cleanupDBDir(dir, name) assert.NotEmpty(t, db.Stats())