From 442ebe1c66c27d8c19d4f0b257200b501066e385 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Mon, 18 May 2020 13:08:35 +0200 Subject: [PATCH] batch: return errors for all operations (#96) Modify batch `Set`, `Delete`, and `Close` to return errors. The main error condition currently is using closed batches, but future ones include databases which implement batches as transactions (e.g. BadgerDB) and rejecting empty keys (#2). --- CHANGELOG.md | 6 +++++ backend_test.go | 51 ++++++++++++++++++++------------------- boltdb_batch.go | 28 +++++++++++---------- cleveldb_batch.go | 32 +++++++++++++----------- goleveldb_batch.go | 28 +++++++++++---------- memdb_batch.go | 28 +++++++++++---------- prefixdb_batch.go | 12 ++++----- remotedb/batch.go | 34 +++++++++++++++----------- remotedb/grpcdb/server.go | 10 ++++++-- remotedb/remotedb_test.go | 32 ++++++++++++------------ rocksdb_batch.go | 32 +++++++++++++----------- types.go | 33 +++++++++++++------------ 12 files changed, 182 insertions(+), 144 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0c60ba46..03501ac97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +### Breaking Changes + +- Batch `Set()`, `Delete()`, and `Close()` may now error (@erikgrinaker) + +- The `SetDeleter` interface has been removed (@erikgrinaker) + ## 0.5.1 **2020-03-30** diff --git a/backend_test.go b/backend_test.go index a7b877bcf..1b0e14200 100644 --- a/backend_test.go +++ b/backend_test.go @@ -449,46 +449,47 @@ func testDBBatch(t *testing.T, backend BackendType) { // create a new batch, and some items - they should not be visible until we write batch := db.NewBatch() - batch.Set([]byte("a"), []byte{1}) - batch.Set([]byte("b"), []byte{2}) - batch.Set([]byte("c"), []byte{3}) + require.NoError(t, batch.Set([]byte("a"), []byte{1})) + require.NoError(t, batch.Set([]byte("b"), []byte{2})) + require.NoError(t, batch.Set([]byte("c"), []byte{3})) assertKeyValues(t, db, map[string][]byte{}) err := batch.Write() require.NoError(t, err) assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}, "c": {3}}) - // trying to modify or rewrite a written batch should panic, but closing it should work - require.Panics(t, func() { batch.Set([]byte("a"), []byte{9}) }) - require.Panics(t, func() { batch.Delete([]byte("a")) }) - require.Panics(t, func() { batch.Write() }) // nolint: errcheck - require.Panics(t, func() { batch.WriteSync() }) // nolint: errcheck - batch.Close() + // trying to modify or rewrite a written batch should error, but closing it should work + require.Error(t, batch.Set([]byte("a"), []byte{9})) + require.Error(t, batch.Delete([]byte("a"))) + require.Error(t, batch.Write()) + require.Error(t, batch.WriteSync()) + require.NoError(t, batch.Close()) // batches should write changes in order batch = db.NewBatch() - batch.Delete([]byte("a")) - batch.Set([]byte("a"), []byte{1}) - batch.Set([]byte("b"), []byte{1}) - batch.Set([]byte("b"), []byte{2}) - batch.Set([]byte("c"), []byte{3}) - batch.Delete([]byte("c")) - err = batch.Write() - require.NoError(t, err) - batch.Close() + require.NoError(t, batch.Delete([]byte("a"))) + require.NoError(t, batch.Set([]byte("a"), []byte{1})) + require.NoError(t, batch.Set([]byte("b"), []byte{1})) + require.NoError(t, batch.Set([]byte("b"), []byte{2})) + require.NoError(t, batch.Set([]byte("c"), []byte{3})) + require.NoError(t, batch.Delete([]byte("c"))) + require.NoError(t, batch.Write()) + require.NoError(t, batch.Close()) assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}}) // writing nil keys and values should be the same as empty keys and values // FIXME CLevelDB panics here: https://github.com/jmhodges/levigo/issues/55 if backend != CLevelDBBackend { batch = db.NewBatch() - batch.Set(nil, nil) + err = batch.Set(nil, nil) + require.NoError(t, err) err = batch.WriteSync() require.NoError(t, err) assertKeyValues(t, db, map[string][]byte{"": {}, "a": {1}, "b": {2}}) batch = db.NewBatch() - batch.Delete(nil) + err = batch.Delete(nil) + require.NoError(t, err) err = batch.Write() require.NoError(t, err) assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}}) @@ -505,11 +506,11 @@ func testDBBatch(t *testing.T, backend BackendType) { batch.Close() batch.Close() - // all other operations on a closed batch should panic - require.Panics(t, func() { batch.Set([]byte("a"), []byte{9}) }) - require.Panics(t, func() { batch.Delete([]byte("a")) }) - require.Panics(t, func() { batch.Write() }) // nolint: errcheck - require.Panics(t, func() { batch.WriteSync() }) // nolint: errcheck + // all other operations on a closed batch should error + require.Error(t, batch.Set([]byte("a"), []byte{9})) + require.Error(t, batch.Delete([]byte("a"))) + require.Error(t, batch.Write()) + require.Error(t, batch.WriteSync()) } func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) { diff --git a/boltdb_batch.go b/boltdb_batch.go index ae0f40b35..ed99ba46a 100644 --- a/boltdb_batch.go +++ b/boltdb_batch.go @@ -19,27 +19,29 @@ func newBoltDBBatch(db *BoltDB) *boltDBBatch { } } -func (b *boltDBBatch) assertOpen() { +// Set implements Batch. +func (b *boltDBBatch) Set(key, value []byte) error { if b.ops == nil { - panic("batch has been written or closed") + return errBatchClosed } -} - -// Set implements Batch. -func (b *boltDBBatch) Set(key, value []byte) { - b.assertOpen() b.ops = append(b.ops, operation{opTypeSet, key, value}) + return nil } // Delete implements Batch. -func (b *boltDBBatch) Delete(key []byte) { - b.assertOpen() +func (b *boltDBBatch) Delete(key []byte) error { + if b.ops == nil { + return errBatchClosed + } b.ops = append(b.ops, operation{opTypeDelete, key, nil}) + return nil } // Write implements Batch. func (b *boltDBBatch) Write() error { - b.assertOpen() + if b.ops == nil { + return errBatchClosed + } err := b.db.db.Batch(func(tx *bbolt.Tx) error { bkt := tx.Bucket(bucket) for _, op := range b.ops { @@ -61,8 +63,7 @@ func (b *boltDBBatch) Write() error { return err } // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. - b.Close() - return nil + return b.Close() } // WriteSync implements Batch. @@ -71,6 +72,7 @@ func (b *boltDBBatch) WriteSync() error { } // Close implements Batch. -func (b *boltDBBatch) Close() { +func (b *boltDBBatch) Close() error { b.ops = nil + return nil } diff --git a/cleveldb_batch.go b/cleveldb_batch.go index eeb2770d2..85faed809 100644 --- a/cleveldb_batch.go +++ b/cleveldb_batch.go @@ -17,39 +17,42 @@ func newCLevelDBBatch(db *CLevelDB) *cLevelDBBatch { } } -func (b *cLevelDBBatch) assertOpen() { +// Set implements Batch. +func (b *cLevelDBBatch) Set(key, value []byte) error { if b.batch == nil { - panic("batch has been written or closed") + return errBatchClosed } -} - -// Set implements Batch. -func (b *cLevelDBBatch) Set(key, value []byte) { - b.assertOpen() b.batch.Put(nonNilBytes(key), nonNilBytes(value)) + return nil } // Delete implements Batch. -func (b *cLevelDBBatch) Delete(key []byte) { - b.assertOpen() +func (b *cLevelDBBatch) Delete(key []byte) error { + if b.batch == nil { + return errBatchClosed + } b.batch.Delete(nonNilBytes(key)) + return nil } // Write implements Batch. func (b *cLevelDBBatch) Write() error { - b.assertOpen() + if b.batch == nil { + return errBatchClosed + } err := b.db.db.Write(b.db.wo, b.batch) if err != nil { return err } // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. - b.Close() - return nil + return b.Close() } // WriteSync implements Batch. func (b *cLevelDBBatch) WriteSync() error { - b.assertOpen() + if b.batch == nil { + return errBatchClosed + } err := b.db.db.Write(b.db.woSync, b.batch) if err != nil { return err @@ -60,9 +63,10 @@ func (b *cLevelDBBatch) WriteSync() error { } // Close implements Batch. -func (b *cLevelDBBatch) Close() { +func (b *cLevelDBBatch) Close() error { if b.batch != nil { b.batch.Close() b.batch = nil } + return nil } diff --git a/goleveldb_batch.go b/goleveldb_batch.go index efb33162a..c4f617360 100644 --- a/goleveldb_batch.go +++ b/goleveldb_batch.go @@ -19,22 +19,22 @@ func newGoLevelDBBatch(db *GoLevelDB) *goLevelDBBatch { } } -func (b *goLevelDBBatch) assertOpen() { +// Set implements Batch. +func (b *goLevelDBBatch) Set(key, value []byte) error { if b.batch == nil { - panic("batch has been written or closed") + return errBatchClosed } -} - -// Set implements Batch. -func (b *goLevelDBBatch) Set(key, value []byte) { - b.assertOpen() b.batch.Put(key, value) + return nil } // Delete implements Batch. -func (b *goLevelDBBatch) Delete(key []byte) { - b.assertOpen() +func (b *goLevelDBBatch) Delete(key []byte) error { + if b.batch == nil { + return errBatchClosed + } b.batch.Delete(key) + return nil } // Write implements Batch. @@ -48,20 +48,22 @@ func (b *goLevelDBBatch) WriteSync() error { } func (b *goLevelDBBatch) write(sync bool) error { - b.assertOpen() + if b.batch == nil { + return errBatchClosed + } err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: sync}) if err != nil { return err } // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. - b.Close() - return nil + return b.Close() } // Close implements Batch. -func (b *goLevelDBBatch) Close() { +func (b *goLevelDBBatch) Close() error { if b.batch != nil { b.batch.Reset() b.batch = nil } + return nil } diff --git a/memdb_batch.go b/memdb_batch.go index d9b94999c..f308ed20e 100644 --- a/memdb_batch.go +++ b/memdb_batch.go @@ -32,27 +32,29 @@ func newMemDBBatch(db *MemDB) *memDBBatch { } } -func (b *memDBBatch) assertOpen() { +// Set implements Batch. +func (b *memDBBatch) Set(key, value []byte) error { if b.ops == nil { - panic("batch has been written or closed") + return errBatchClosed } -} - -// Set implements Batch. -func (b *memDBBatch) Set(key, value []byte) { - b.assertOpen() b.ops = append(b.ops, operation{opTypeSet, key, value}) + return nil } // Delete implements Batch. -func (b *memDBBatch) Delete(key []byte) { - b.assertOpen() +func (b *memDBBatch) Delete(key []byte) error { + if b.ops == nil { + return errBatchClosed + } b.ops = append(b.ops, operation{opTypeDelete, key, nil}) + return nil } // Write implements Batch. func (b *memDBBatch) Write() error { - b.assertOpen() + if b.ops == nil { + return errBatchClosed + } b.db.mtx.Lock() defer b.db.mtx.Unlock() @@ -68,8 +70,7 @@ func (b *memDBBatch) Write() error { } // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. - b.Close() - return nil + return b.Close() } // WriteSync implements Batch. @@ -78,6 +79,7 @@ func (b *memDBBatch) WriteSync() error { } // Close implements Batch. -func (b *memDBBatch) Close() { +func (b *memDBBatch) Close() error { b.ops = nil + return nil } diff --git a/prefixdb_batch.go b/prefixdb_batch.go index a3547de18..73433c2ee 100644 --- a/prefixdb_batch.go +++ b/prefixdb_batch.go @@ -15,15 +15,15 @@ func newPrefixBatch(prefix []byte, source Batch) prefixDBBatch { } // Set implements Batch. -func (pb prefixDBBatch) Set(key, value []byte) { +func (pb prefixDBBatch) Set(key, value []byte) error { pkey := append(cp(pb.prefix), key...) - pb.source.Set(pkey, value) + return pb.source.Set(pkey, value) } // Delete implements Batch. -func (pb prefixDBBatch) Delete(key []byte) { +func (pb prefixDBBatch) Delete(key []byte) error { pkey := append(cp(pb.prefix), key...) - pb.source.Delete(pkey) + return pb.source.Delete(pkey) } // Write implements Batch. @@ -37,6 +37,6 @@ func (pb prefixDBBatch) WriteSync() error { } // Close implements Batch. -func (pb prefixDBBatch) Close() { - pb.source.Close() +func (pb prefixDBBatch) Close() error { + return pb.source.Close() } diff --git a/remotedb/batch.go b/remotedb/batch.go index 8187fd7f3..68b5f17bd 100644 --- a/remotedb/batch.go +++ b/remotedb/batch.go @@ -7,6 +7,8 @@ import ( protodb "github.com/tendermint/tm-db/remotedb/proto" ) +var errBatchClosed = errors.New("batch has been written or closed") + type batch struct { db *RemoteDB ops []*protodb.Operation @@ -21,35 +23,37 @@ func newBatch(rdb *RemoteDB) *batch { } } -func (b *batch) assertOpen() { +// Set implements Batch. +func (b *batch) Set(key, value []byte) error { if b.ops == nil { - panic("batch has been written or closed") + return errBatchClosed } -} - -// Set implements Batch. -func (b *batch) Set(key, value []byte) { - b.assertOpen() op := &protodb.Operation{ Entity: &protodb.Entity{Key: key, Value: value}, Type: protodb.Operation_SET, } b.ops = append(b.ops, op) + return nil } // Delete implements Batch. -func (b *batch) Delete(key []byte) { - b.assertOpen() +func (b *batch) Delete(key []byte) error { + if b.ops == nil { + return errBatchClosed + } op := &protodb.Operation{ Entity: &protodb.Entity{Key: key}, Type: protodb.Operation_DELETE, } b.ops = append(b.ops, op) + return nil } // Write implements Batch. func (b *batch) Write() error { - b.assertOpen() + if b.ops == nil { + return errBatchClosed + } _, err := b.db.dc.BatchWrite(b.db.ctx, &protodb.Batch{Ops: b.ops}) if err != nil { return errors.Errorf("remoteDB.BatchWrite: %v", err) @@ -61,17 +65,19 @@ func (b *batch) Write() error { // WriteSync implements Batch. func (b *batch) WriteSync() error { - b.assertOpen() + if b.ops == nil { + return errBatchClosed + } _, err := b.db.dc.BatchWriteSync(b.db.ctx, &protodb.Batch{Ops: b.ops}) if err != nil { return errors.Errorf("RemoteDB.BatchWriteSync: %v", err) } // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. - b.Close() - return nil + return b.Close() } // Close implements Batch. -func (b *batch) Close() { +func (b *batch) Close() error { b.ops = nil + return nil } diff --git a/remotedb/grpcdb/server.go b/remotedb/grpcdb/server.go index 7c3431d83..79dc78646 100644 --- a/remotedb/grpcdb/server.go +++ b/remotedb/grpcdb/server.go @@ -214,9 +214,15 @@ func (s *server) batchWrite(c context.Context, b *protodb.Batch, sync bool) (*pr for _, op := range b.Ops { switch op.Type { case protodb.Operation_SET: - bat.Set(op.Entity.Key, op.Entity.Value) + err := bat.Set(op.Entity.Key, op.Entity.Value) + if err != nil { + return nil, err + } case protodb.Operation_DELETE: - bat.Delete(op.Entity.Key) + err := bat.Delete(op.Entity.Key) + if err != nil { + return nil, err + } } } if sync { diff --git a/remotedb/remotedb_test.go b/remotedb/remotedb_test.go index c8b866e0c..01af129e4 100644 --- a/remotedb/remotedb_test.go +++ b/remotedb/remotedb_test.go @@ -19,9 +19,9 @@ func TestRemoteDB(t *testing.T) { srv, err := grpcdb.NewServer(cert, key) require.Nil(t, err) defer srv.Stop() - go func() { //nolint:staticcheck + go func() { if err := srv.Serve(ln); err != nil { - t.Fatalf("BindServer: %v", err) + panic(err) } }() @@ -29,12 +29,7 @@ func TestRemoteDB(t *testing.T) { require.Nil(t, err, "expecting a successful client creation") dbName := "test-remote-db" require.Nil(t, client.InitRemote(&remotedb.Init{Name: dbName, Type: "goleveldb"})) - defer func() { - err := os.RemoveAll(dbName + ".db") - if err != nil { - panic(err) - } - }() + defer os.RemoveAll(dbName + ".db") k1 := []byte("key-1") v1, err := client.Get(k1) @@ -114,8 +109,10 @@ func TestRemoteDB(t *testing.T) { v4 := []byte("value-4") v5 := []byte("value-5") bat := client.NewBatch() - bat.Set(k3, v3) - bat.Set(k4, v4) + err = bat.Set(k3, v3) + require.NoError(t, err) + err = bat.Set(k4, v4) + require.NoError(t, err) rv3, err := client.Get(k3) require.NoError(t, err) @@ -137,8 +134,10 @@ func TestRemoteDB(t *testing.T) { // Batch tests - deletion bat = client.NewBatch() - bat.Delete(k4) - bat.Delete(k3) + err = bat.Delete(k4) + require.NoError(t, err) + err = bat.Delete(k3) + require.NoError(t, err) err = bat.WriteSync() require.NoError(t, err) @@ -152,9 +151,12 @@ func TestRemoteDB(t *testing.T) { // Batch tests - set and delete bat = client.NewBatch() - bat.Set(k4, v4) - bat.Set(k5, v5) - bat.Delete(k4) + err = bat.Set(k4, v4) + require.NoError(t, err) + err = bat.Set(k5, v5) + require.NoError(t, err) + err = bat.Delete(k4) + require.NoError(t, err) err = bat.WriteSync() require.NoError(t, err) diff --git a/rocksdb_batch.go b/rocksdb_batch.go index 9cab3df6d..022e537b9 100644 --- a/rocksdb_batch.go +++ b/rocksdb_batch.go @@ -18,27 +18,29 @@ func newRocksDBBatch(db *RocksDB) *rocksDBBatch { } } -func (b *rocksDBBatch) assertOpen() { +// Set implements Batch. +func (b *rocksDBBatch) Set(key, value []byte) error { if b.batch == nil { - panic("batch has been written or closed") + return errBatchClosed } -} - -// Set implements Batch. -func (b *rocksDBBatch) Set(key, value []byte) { - b.assertOpen() b.batch.Put(key, value) + return nil } // Delete implements Batch. -func (b *rocksDBBatch) Delete(key []byte) { - b.assertOpen() +func (b *rocksDBBatch) Delete(key []byte) error { + if b.batch == nil { + return errBatchClosed + } b.batch.Delete(key) + return nil } // Write implements Batch. func (b *rocksDBBatch) Write() error { - b.assertOpen() + if b.batch == nil { + return errBatchClosed + } err := b.db.db.Write(b.db.wo, b.batch) if err != nil { return err @@ -50,20 +52,22 @@ func (b *rocksDBBatch) Write() error { // WriteSync implements Batch. func (b *rocksDBBatch) WriteSync() error { - b.assertOpen() + if b.batch == nil { + return errBatchClosed + } err := b.db.db.Write(b.db.woSync, b.batch) if err != nil { return err } // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. - b.Close() - return nil + return b.Close() } // Close implements Batch. -func (b *rocksDBBatch) Close() { +func (b *rocksDBBatch) Close() error { if b.batch != nil { b.batch.Destroy() b.batch = nil } + return nil } diff --git a/types.go b/types.go index c5828ec2b..6a373293b 100644 --- a/types.go +++ b/types.go @@ -1,5 +1,12 @@ package db +import "errors" + +var ( + // errBatchClosed is returned when a closed or written batch is used. + errBatchClosed = errors.New("batch has been written or closed") +) + // DB is the main interface for all database backends. DBs are concurrency-safe. Callers must call // Close on the database when done. // @@ -62,28 +69,24 @@ type DB interface { // As with DB, given keys and values should be considered read-only, and must not be modified after // passing them to the batch. type Batch interface { - SetDeleter + // Set sets a key/value pair. + // CONTRACT: key, value readonly []byte + Set(key, value []byte) error + + // Delete deletes a key/value pair. + // CONTRACT: key readonly []byte + Delete(key []byte) error // Write writes the batch, possibly without flushing to disk. Only Close() can be called after, - // other methods will panic. + // other methods will error. Write() error // WriteSync writes the batch and flushes it to disk. Only Close() can be called after, other - // methods will panic. + // methods will error. WriteSync() error - // Close closes the batch. It is idempotent, but any other calls afterwards will panic. - Close() -} - -type SetDeleter interface { - // Set sets a key/value pair. - // CONTRACT: key, value readonly []byte - Set(key, value []byte) - - // Delete deletes a key/value pair. - // CONTRACT: key readonly []byte - Delete(key []byte) + // Close closes the batch. It is idempotent, but calls to other methods afterwards will error. + Close() error } // Iterator represents an iterator over a domain of keys. Callers must call Close when done.