From f461790ef23cbeed7166c0bfbaa17fe109167d6a Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:05:18 +0100 Subject: [PATCH 01/17] Batch interface returns errors --- CHANGELOG.md | 4 ++++ boltdb_batch.go | 10 +++++++--- cleveldb_batch.go | 9 ++++++--- goleveldb_batch.go | 9 ++++++--- memdb_batch.go | 9 ++++++--- prefixdb_batch.go | 9 ++++++--- remotedb/batch.go | 9 ++++++--- rocksdb_batch.go | 21 ++++++++------------- types.go | 27 ++++++++++++++------------- 9 files changed, 63 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d89f6e047..da362ad98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ ### Breaking Changes +- `Batch` interface methods now return errors + +- Removed the `SetDeleter` interface + - [memdb] [\#56](https://github.com/tendermint/tm-db/pull/56) Removed some exported methods that were mainly meant for internal use: `Mutex()`, `SetNoLock()`, `SetNoLockSync()`, `DeleteNoLock()`, and `DeleteNoLockSync()` ### Improvements diff --git a/boltdb_batch.go b/boltdb_batch.go index a5996fe38..512c71650 100644 --- a/boltdb_batch.go +++ b/boltdb_batch.go @@ -13,13 +13,15 @@ type boltDBBatch struct { var _ Batch = (*boltDBBatch)(nil) // Set implements Batch. -func (b *boltDBBatch) Set(key, value []byte) { +func (b *boltDBBatch) Set(key, value []byte) error { b.ops = append(b.ops, operation{opTypeSet, key, value}) + return nil } // Delete implements Batch. -func (b *boltDBBatch) Delete(key []byte) { +func (b *boltDBBatch) Delete(key []byte) error { b.ops = append(b.ops, operation{opTypeDelete, key, nil}) + return nil } // Write implements Batch. @@ -49,4 +51,6 @@ func (b *boltDBBatch) WriteSync() error { } // Close implements Batch. -func (b *boltDBBatch) Close() {} +func (b *boltDBBatch) Close() error { + return nil +} diff --git a/cleveldb_batch.go b/cleveldb_batch.go index c98fe3354..fce2883fb 100644 --- a/cleveldb_batch.go +++ b/cleveldb_batch.go @@ -11,13 +11,15 @@ type cLevelDBBatch struct { } // Set implements Batch. -func (b *cLevelDBBatch) Set(key, value []byte) { +func (b *cLevelDBBatch) Set(key, value []byte) error { b.batch.Put(key, value) + return nil } // Delete implements Batch. -func (b *cLevelDBBatch) Delete(key []byte) { +func (b *cLevelDBBatch) Delete(key []byte) error { b.batch.Delete(key) + return nil } // Write implements Batch. @@ -37,6 +39,7 @@ func (b *cLevelDBBatch) WriteSync() error { } // Close implements Batch. -func (b *cLevelDBBatch) Close() { +func (b *cLevelDBBatch) Close() error { b.batch.Close() + return nil } diff --git a/goleveldb_batch.go b/goleveldb_batch.go index ec290fe10..caac71edb 100644 --- a/goleveldb_batch.go +++ b/goleveldb_batch.go @@ -13,13 +13,15 @@ type goLevelDBBatch struct { var _ Batch = (*goLevelDBBatch)(nil) // Set implements Batch. -func (b *goLevelDBBatch) Set(key, value []byte) { +func (b *goLevelDBBatch) Set(key, value []byte) error { b.batch.Put(key, value) + return nil } // Delete implements Batch. -func (b *goLevelDBBatch) Delete(key []byte) { +func (b *goLevelDBBatch) Delete(key []byte) error { b.batch.Delete(key) + return nil } // Write implements Batch. @@ -41,6 +43,7 @@ func (b *goLevelDBBatch) WriteSync() error { } // Close implements Batch. -func (b *goLevelDBBatch) Close() { +func (b *goLevelDBBatch) Close() error { b.batch.Reset() + return nil } diff --git a/memdb_batch.go b/memdb_batch.go index 4bd1f1d0c..6d0b47433 100644 --- a/memdb_batch.go +++ b/memdb_batch.go @@ -25,13 +25,15 @@ type memDBBatch struct { var _ Batch = (*memDBBatch)(nil) // Set implements Batch. -func (b *memDBBatch) Set(key, value []byte) { +func (b *memDBBatch) Set(key, value []byte) error { b.ops = append(b.ops, operation{opTypeSet, key, value}) + return nil } // Delete implements Batch. -func (b *memDBBatch) Delete(key []byte) { +func (b *memDBBatch) Delete(key []byte) error { b.ops = append(b.ops, operation{opTypeDelete, key, nil}) + return nil } // Write implements Batch. @@ -58,6 +60,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..6e9437920 100644 --- a/prefixdb_batch.go +++ b/prefixdb_batch.go @@ -15,15 +15,17 @@ 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 nil } // 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 nil } // Write implements Batch. @@ -37,6 +39,7 @@ func (pb prefixDBBatch) WriteSync() error { } // Close implements Batch. -func (pb prefixDBBatch) Close() { +func (pb prefixDBBatch) Close() error { pb.source.Close() + return nil } diff --git a/remotedb/batch.go b/remotedb/batch.go index 5fb92f30c..c93cf2e59 100644 --- a/remotedb/batch.go +++ b/remotedb/batch.go @@ -15,21 +15,23 @@ type batch struct { var _ db.Batch = (*batch)(nil) // Set implements Batch. -func (b *batch) Set(key, value []byte) { +func (b *batch) Set(key, value []byte) error { 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) { +func (b *batch) Delete(key []byte) error { op := &protodb.Operation{ Entity: &protodb.Entity{Key: key}, Type: protodb.Operation_DELETE, } b.ops = append(b.ops, op) + return nil } // Write implements Batch. @@ -49,6 +51,7 @@ func (b *batch) WriteSync() error { } // Close implements Batch. -func (b *batch) Close() { +func (b *batch) Close() error { b.ops = nil + return nil } diff --git a/rocksdb_batch.go b/rocksdb_batch.go index 085ec51ce..92befd550 100644 --- a/rocksdb_batch.go +++ b/rocksdb_batch.go @@ -12,34 +12,29 @@ type rocksDBBatch struct { var _ Batch = (*rocksDBBatch)(nil) // Set implements Batch. -func (mBatch *rocksDBBatch) Set(key, value []byte) { +func (mBatch *rocksDBBatch) Set(key, value []byte) error { mBatch.batch.Put(key, value) + return nil } // Delete implements Batch. -func (mBatch *rocksDBBatch) Delete(key []byte) { +func (mBatch *rocksDBBatch) Delete(key []byte) error { mBatch.batch.Delete(key) + return nil } // Write implements Batch. func (mBatch *rocksDBBatch) Write() error { - err := mBatch.db.db.Write(mBatch.db.wo, mBatch.batch) - if err != nil { - return err - } - return nil + return mBatch.db.db.Write(mBatch.db.wo, mBatch.batch) } // WriteSync mplements Batch. func (mBatch *rocksDBBatch) WriteSync() error { - err := mBatch.db.db.Write(mBatch.db.woSync, mBatch.batch) - if err != nil { - return err - } - return nil + return mBatch.db.db.Write(mBatch.db.woSync, mBatch.batch) } // Close implements Batch. -func (mBatch *rocksDBBatch) Close() { +func (mBatch *rocksDBBatch) Close() error { mBatch.batch.Destroy() + return nil } diff --git a/types.go b/types.go index 1de83e96f..9081b8d5d 100644 --- a/types.go +++ b/types.go @@ -44,7 +44,7 @@ type DB interface { // Closes the connection. Close() error - // Creates a batch for atomic updates. + // Creates a batch for atomic updates. The caller must call Batch.Close. NewBatch() Batch // For debugging @@ -54,25 +54,26 @@ type DB interface { Stats() map[string]string } -//---------------------------------------- -// Batch - // Batch Close must be called when the program no longer needs the object. type Batch interface { - SetDeleter + // Set sets a key/value pair. Key and value cannot be nil. + // CONTRACT: key, value readonly []byte + Set(key, value []byte) error + + // Delete deles a key. Key cannot be nil. + // CONTRACT: key readonly []byte + Delete(key []byte) error + + // Write writes the batch, possibly without flushing to disk. Write() error + + // WriteSync writes the batch and flushes it to disk. WriteSync() error - Close() -} -type SetDeleter interface { - Set(key, value []byte) // CONTRACT: key, value readonly []byte - Delete(key []byte) // CONTRACT: key readonly []byte + // Close closes the batch. + Close() error } -//---------------------------------------- -// Iterator - /* Usage: From a52e1a4bffba67236563fbeeafb256e3f07bb023 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:22:45 +0100 Subject: [PATCH 02/17] Added tests for new batch semantics --- backend_test.go | 75 +++++++++++++++++++++++-------------------------- types.go | 16 +++++++---- 2 files changed, 46 insertions(+), 45 deletions(-) diff --git a/backend_test.go b/backend_test.go index 423d7ba77..80e1577ea 100644 --- a/backend_test.go +++ b/backend_test.go @@ -430,69 +430,64 @@ 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}}) - // the batch still keeps these values internally, so changing values and rewriting batch - // should set the values again - err = db.Set([]byte("a"), []byte{9}) - require.NoError(t, err) - err = db.Delete([]byte("c")) - require.NoError(t, err) + // writing a batch should implicitly close it, so writing it again should error err = batch.WriteSync() - require.NoError(t, err) - assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}, "c": {3}}) + require.Equal(t, errBatchClosed, err) - // but when we close, it should no longer set the values - batch.Close() - err = db.Delete([]byte("c")) - require.NoError(t, err) - err = batch.Write() - require.NoError(t, err) + // batches should write changes in order + batch = db.NewBatch() + 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}}) - // it should be possible to re-close the batch - batch.Close() - - // it should also be possible to reuse a closed batch as if it were a new one - batch.Set([]byte("c"), []byte{3}) - err = batch.Write() + // writing nil keys and values should be the same as empty keys and values + batch = db.NewBatch() + err = batch.Set(nil, nil) require.NoError(t, err) - assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}, "c": {3}}) - batch.Close() - - batch.Delete([]byte("c")) err = batch.WriteSync() require.NoError(t, err) - assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}}) - batch.Close() + assertKeyValues(t, db, map[string][]byte{"": {}, "a": {1}, "b": {2}}) - // batches should also 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.Delete(nil) + require.NoError(t, err) err = batch.Write() require.NoError(t, err) - batch.Close() assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}}) - // and writing an empty batch should not fail + // it should be possible to write an empty batch batch = db.NewBatch() err = batch.Write() require.NoError(t, err) - err = batch.WriteSync() - require.NoError(t, err) assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}}) + + // it should be possible to close an empty batch, and to re-close a closed batch + batch = db.NewBatch() + err = batch.Close() + require.NoError(t, err) + err = batch.Close() + require.NoError(t, err) + + // all other operations on a closed batch should error + require.Equal(t, errBatchClosed, batch.Set([]byte("a"), []byte{9})) + require.Equal(t, errBatchClosed, batch.Delete([]byte("a"))) + require.Equal(t, errBatchClosed, batch.Write()) + require.Equal(t, errBatchClosed, batch.WriteSync()) } func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) { diff --git a/types.go b/types.go index 9081b8d5d..5198a12a2 100644 --- a/types.go +++ b/types.go @@ -1,5 +1,11 @@ package db +import "github.com/pkg/errors" + +var ( + errBatchClosed = errors.New("batch is closed") +) + // DBs are goroutine safe. type DB interface { @@ -56,21 +62,21 @@ type DB interface { // Batch Close must be called when the program no longer needs the object. type Batch interface { - // Set sets a key/value pair. Key and value cannot be nil. + // Set sets a key/value pair. // CONTRACT: key, value readonly []byte Set(key, value []byte) error - // Delete deles a key. Key cannot be nil. + // Delete deles a key. // CONTRACT: key readonly []byte Delete(key []byte) error - // Write writes the batch, possibly without flushing to disk. + // Write writes the batch, possibly without flushing to disk. It implicitly closes the batch. Write() error - // WriteSync writes the batch and flushes it to disk. + // WriteSync writes the batch and flushes it to disk. It implicitly closes the batch. WriteSync() error - // Close closes the batch. + // Close closes the batch. It is idempotent, but the batch cannot be used afterwards. Close() error } From d2cd3a9425442953e789d1256619dd1de1c3de85 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:22:57 +0100 Subject: [PATCH 03/17] Ported MemDB to new batch interface --- memdb.go | 2 +- memdb_batch.go | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/memdb.go b/memdb.go index 9cbb99ab8..68acaf29e 100644 --- a/memdb.go +++ b/memdb.go @@ -150,7 +150,7 @@ func (db *MemDB) Stats() map[string]string { // NewBatch implements DB. func (db *MemDB) NewBatch() Batch { - return &memDBBatch{db, nil} + return newMemDBBatch(db) } // Iterator implements DB. diff --git a/memdb_batch.go b/memdb_batch.go index 6d0b47433..18b2baf83 100644 --- a/memdb_batch.go +++ b/memdb_batch.go @@ -24,20 +24,37 @@ type memDBBatch struct { var _ Batch = (*memDBBatch)(nil) +// newMemDBBatch creates a new memDBBatch +func newMemDBBatch(db *MemDB) *memDBBatch { + return &memDBBatch{ + db: db, + ops: make([]operation, 0, 1), + } +} + // Set implements Batch. func (b *memDBBatch) Set(key, value []byte) error { + if b.ops == nil { + return errBatchClosed + } b.ops = append(b.ops, operation{opTypeSet, key, value}) return nil } // Delete implements Batch. 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 { + if b.ops == nil { + return errBatchClosed + } b.db.mtx.Lock() defer b.db.mtx.Unlock() @@ -51,7 +68,7 @@ func (b *memDBBatch) Write() error { return errors.Errorf("unknown operation type %v (%v)", op.opType, op) } } - return nil + return b.Close() } // WriteSync implements Batch. From de5a944252177b9c923df17d1b6a12ccec1a00bf Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:28:59 +0100 Subject: [PATCH 04/17] Ported GoLevelDB to new batch interface --- goleveldb.go | 3 +-- goleveldb_batch.go | 28 +++++++++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/goleveldb.go b/goleveldb.go index 0c54cdee9..eadc83606 100644 --- a/goleveldb.go +++ b/goleveldb.go @@ -153,8 +153,7 @@ func (db *GoLevelDB) Stats() map[string]string { // NewBatch implements DB. func (db *GoLevelDB) NewBatch() Batch { - batch := new(leveldb.Batch) - return &goLevelDBBatch{db, batch} + return newGoLevelDBBatch(db) } // Iterator implements DB. diff --git a/goleveldb_batch.go b/goleveldb_batch.go index caac71edb..aa66cb85c 100644 --- a/goleveldb_batch.go +++ b/goleveldb_batch.go @@ -12,38 +12,60 @@ type goLevelDBBatch struct { var _ Batch = (*goLevelDBBatch)(nil) +func newGoLevelDBBatch(db *GoLevelDB) *goLevelDBBatch { + return &goLevelDBBatch{ + db: db, + batch: new(leveldb.Batch), + } +} + // Set implements Batch. func (b *goLevelDBBatch) Set(key, value []byte) error { + if b.batch == nil { + return errBatchClosed + } b.batch.Put(key, value) return nil } // Delete implements Batch. func (b *goLevelDBBatch) Delete(key []byte) error { + if b.batch == nil { + return errBatchClosed + } b.batch.Delete(key) return nil } // Write implements Batch. func (b *goLevelDBBatch) Write() error { + if b.batch == nil { + return errBatchClosed + } err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: false}) if err != nil { return err } - return nil + return b.Close() } // WriteSync implements Batch. func (b *goLevelDBBatch) WriteSync() error { + if b.batch == nil { + return errBatchClosed + } err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: true}) if err != nil { return err } - return nil + return b.Close() } // Close implements Batch. func (b *goLevelDBBatch) Close() error { - b.batch.Reset() + if b.batch != nil { + b.batch.Reset() + b.batch = nil + } return nil } From 5e27f010a236535278ef7ea919a2980463ecd35c Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:32:25 +0100 Subject: [PATCH 05/17] Ported BoltDB to new batch interface --- boltdb.go | 5 +---- boltdb_batch.go | 23 ++++++++++++++++++++++- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/boltdb.go b/boltdb.go index 522a43b51..8360a6842 100644 --- a/boltdb.go +++ b/boltdb.go @@ -169,10 +169,7 @@ func (bdb *BoltDB) Stats() map[string]string { // NewBatch implements DB. func (bdb *BoltDB) NewBatch() Batch { - return &boltDBBatch{ - ops: nil, - db: bdb, - } + return newBoltDBBatch(bdb) } // WARNING: Any concurrent writes or reads will block until the iterator is diff --git a/boltdb_batch.go b/boltdb_batch.go index 512c71650..0089b4211 100644 --- a/boltdb_batch.go +++ b/boltdb_batch.go @@ -12,21 +12,37 @@ type boltDBBatch struct { var _ Batch = (*boltDBBatch)(nil) +func newBoltDBBatch(db *BoltDB) *boltDBBatch { + return &boltDBBatch{ + db: db, + ops: make([]operation, 0, 1), + } +} + // Set implements Batch. func (b *boltDBBatch) Set(key, value []byte) error { + if b.ops == nil { + return errBatchClosed + } b.ops = append(b.ops, operation{opTypeSet, key, value}) return nil } // Delete implements Batch. 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 { - return b.db.db.Batch(func(tx *bbolt.Tx) error { + 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 { key := nonEmptyKey(nonNilBytes(op.key)) @@ -43,6 +59,10 @@ func (b *boltDBBatch) Write() error { } return nil }) + if err != nil { + return err + } + return b.Close() } // WriteSync implements Batch. @@ -52,5 +72,6 @@ func (b *boltDBBatch) WriteSync() error { // Close implements Batch. func (b *boltDBBatch) Close() error { + b.ops = nil return nil } From 06ff8023c4d7bc554d081b96bfe8c7b360677e4c Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:38:35 +0100 Subject: [PATCH 06/17] Ported CLevelDB to new batch interface --- cleveldb.go | 3 +-- cleveldb_batch.go | 34 +++++++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/cleveldb.go b/cleveldb.go index 066126c1f..e2e983557 100644 --- a/cleveldb.go +++ b/cleveldb.go @@ -159,8 +159,7 @@ func (db *CLevelDB) Stats() map[string]string { // NewBatch implements DB. func (db *CLevelDB) NewBatch() Batch { - batch := levigo.NewWriteBatch() - return &cLevelDBBatch{db, batch} + return newCLevelDBBatch(db) } // Iterator implements DB. diff --git a/cleveldb_batch.go b/cleveldb_batch.go index fce2883fb..85c6e0221 100644 --- a/cleveldb_batch.go +++ b/cleveldb_batch.go @@ -10,36 +10,60 @@ type cLevelDBBatch struct { batch *levigo.WriteBatch } +func newCLevelDBBatch(db *CLevelDB) *cLevelDBBatch { + return &cLevelDBBatch{ + db: db, + batch: levigo.NewWriteBatch(), + } +} + // Set implements Batch. func (b *cLevelDBBatch) Set(key, value []byte) error { + if b.batch == nil { + return errBatchClosed + } b.batch.Put(key, value) return nil } // Delete implements Batch. func (b *cLevelDBBatch) Delete(key []byte) error { + if b.batch == nil { + return errBatchClosed + } b.batch.Delete(key) return nil } // Write implements Batch. func (b *cLevelDBBatch) Write() error { - if err := b.db.db.Write(b.db.wo, b.batch); err != nil { + if b.batch == nil { + return errBatchClosed + } + err := b.db.db.Write(b.db.wo, b.batch) + if err != nil { return err } - return nil + return b.Close() } // WriteSync implements Batch. func (b *cLevelDBBatch) WriteSync() error { - if err := b.db.db.Write(b.db.woSync, b.batch); err != nil { + if b.batch == nil { + return errBatchClosed + } + err := b.db.db.Write(b.db.woSync, b.batch) + if err != nil { return err } - return nil + return b.Close() } // Close implements Batch. func (b *cLevelDBBatch) Close() error { - b.batch.Close() + if b.batch != nil { + b.batch.Close() + b.batch = nil + } return nil } From 50dce04087dbaefe488b0f5f823de1d94c794de0 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:39:49 +0100 Subject: [PATCH 07/17] Ported PrefixDB to new batch interface --- prefixdb_batch.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/prefixdb_batch.go b/prefixdb_batch.go index 6e9437920..73433c2ee 100644 --- a/prefixdb_batch.go +++ b/prefixdb_batch.go @@ -17,15 +17,13 @@ func newPrefixBatch(prefix []byte, source Batch) prefixDBBatch { // Set implements Batch. func (pb prefixDBBatch) Set(key, value []byte) error { pkey := append(cp(pb.prefix), key...) - pb.source.Set(pkey, value) - return nil + return pb.source.Set(pkey, value) } // Delete implements Batch. func (pb prefixDBBatch) Delete(key []byte) error { pkey := append(cp(pb.prefix), key...) - pb.source.Delete(pkey) - return nil + return pb.source.Delete(pkey) } // Write implements Batch. @@ -40,6 +38,5 @@ func (pb prefixDBBatch) WriteSync() error { // Close implements Batch. func (pb prefixDBBatch) Close() error { - pb.source.Close() - return nil + return pb.source.Close() } From 509e3a659fdd5f8b6246f092b7f50b3563a2275b Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:42:57 +0100 Subject: [PATCH 08/17] Ported RocksDB to new batch interface --- rocksdb.go | 3 +-- rocksdb_batch.go | 50 ++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/rocksdb.go b/rocksdb.go index b4bcf1def..e2e23ce7e 100644 --- a/rocksdb.go +++ b/rocksdb.go @@ -164,8 +164,7 @@ func (db *RocksDB) Stats() map[string]string { // NewBatch implements DB. func (db *RocksDB) NewBatch() Batch { - batch := gorocksdb.NewWriteBatch() - return &rocksDBBatch{db, batch} + return newRocksDBBatch(db) } // Iterator implements DB. diff --git a/rocksdb_batch.go b/rocksdb_batch.go index 92befd550..73c662798 100644 --- a/rocksdb_batch.go +++ b/rocksdb_batch.go @@ -11,30 +11,60 @@ type rocksDBBatch struct { var _ Batch = (*rocksDBBatch)(nil) +func newRocksDBBatch(db *RocksDB) *rocksDBBatch { + return &rocksDBBatch{ + db: db, + batch: gorocksdb.NewWriteBatch(), + } +} + // Set implements Batch. -func (mBatch *rocksDBBatch) Set(key, value []byte) error { - mBatch.batch.Put(key, value) +func (b *rocksDBBatch) Set(key, value []byte) error { + if b.batch == nil { + return errBatchClosed + } + b.batch.Put(key, value) return nil } // Delete implements Batch. -func (mBatch *rocksDBBatch) Delete(key []byte) error { - mBatch.batch.Delete(key) +func (b *rocksDBBatch) Delete(key []byte) error { + if b.batch == nil { + return errBatchClosed + } + b.batch.Delete(key) return nil } // Write implements Batch. -func (mBatch *rocksDBBatch) Write() error { - return mBatch.db.db.Write(mBatch.db.wo, mBatch.batch) +func (b *rocksDBBatch) Write() error { + if b.batch == nil { + return errBatchClosed + } + err := b.db.db.Write(b.db.wo, b.batch) + if err != nil { + return err + } + return b.Close() } // WriteSync mplements Batch. -func (mBatch *rocksDBBatch) WriteSync() error { - return mBatch.db.db.Write(mBatch.db.woSync, mBatch.batch) +func (b *rocksDBBatch) WriteSync() error { + if b.batch == nil { + return errBatchClosed + } + err := b.db.db.Write(b.db.woSync, b.batch) + if err != nil { + return err + } + return b.Close() } // Close implements Batch. -func (mBatch *rocksDBBatch) Close() error { - mBatch.batch.Destroy() +func (b *rocksDBBatch) Close() error { + if b.batch != nil { + b.batch.Destroy() + b.batch = nil + } return nil } From 2ba5dce4fe57ddf366d6cac67ed595637b56f66b Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:43:55 +0100 Subject: [PATCH 09/17] Updated changelog --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da362ad98..54ffcee90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ - `Batch` interface methods now return errors +- Closed batches can no longer be reused + - Removed the `SetDeleter` interface - [memdb] [\#56](https://github.com/tendermint/tm-db/pull/56) Removed some exported methods that were mainly meant for internal use: `Mutex()`, `SetNoLock()`, `SetNoLockSync()`, `DeleteNoLock()`, and `DeleteNoLockSync()` @@ -22,8 +24,6 @@ - [cleveldb] Fix handling of empty keys as iterator endpoints -- [goleveldb] [\#58](https://github.com/tendermint/tm-db/pull/58) Make `Batch.Close()` actually remove the batch contents - ## 0.4.1 **2020-2-26** From adde9ca9e3277f9bb3280dc9c58b6c86e6c2d039 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:49:01 +0100 Subject: [PATCH 10/17] Exported ErrBatchClosed --- backend_test.go | 10 +++++----- boltdb_batch.go | 6 +++--- cleveldb_batch.go | 8 ++++---- goleveldb_batch.go | 8 ++++---- memdb_batch.go | 6 +++--- rocksdb_batch.go | 8 ++++---- types.go | 3 ++- 7 files changed, 25 insertions(+), 24 deletions(-) diff --git a/backend_test.go b/backend_test.go index 80e1577ea..4ee6d6617 100644 --- a/backend_test.go +++ b/backend_test.go @@ -441,7 +441,7 @@ func testDBBatch(t *testing.T, backend BackendType) { // writing a batch should implicitly close it, so writing it again should error err = batch.WriteSync() - require.Equal(t, errBatchClosed, err) + require.Equal(t, ErrBatchClosed, err) // batches should write changes in order batch = db.NewBatch() @@ -484,10 +484,10 @@ func testDBBatch(t *testing.T, backend BackendType) { require.NoError(t, err) // all other operations on a closed batch should error - require.Equal(t, errBatchClosed, batch.Set([]byte("a"), []byte{9})) - require.Equal(t, errBatchClosed, batch.Delete([]byte("a"))) - require.Equal(t, errBatchClosed, batch.Write()) - require.Equal(t, errBatchClosed, batch.WriteSync()) + require.Equal(t, ErrBatchClosed, batch.Set([]byte("a"), []byte{9})) + require.Equal(t, ErrBatchClosed, batch.Delete([]byte("a"))) + require.Equal(t, ErrBatchClosed, batch.Write()) + require.Equal(t, ErrBatchClosed, batch.WriteSync()) } func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) { diff --git a/boltdb_batch.go b/boltdb_batch.go index 0089b4211..cb8ed791b 100644 --- a/boltdb_batch.go +++ b/boltdb_batch.go @@ -22,7 +22,7 @@ func newBoltDBBatch(db *BoltDB) *boltDBBatch { // Set implements Batch. func (b *boltDBBatch) Set(key, value []byte) error { if b.ops == nil { - return errBatchClosed + return ErrBatchClosed } b.ops = append(b.ops, operation{opTypeSet, key, value}) return nil @@ -31,7 +31,7 @@ func (b *boltDBBatch) Set(key, value []byte) error { // Delete implements Batch. func (b *boltDBBatch) Delete(key []byte) error { if b.ops == nil { - return errBatchClosed + return ErrBatchClosed } b.ops = append(b.ops, operation{opTypeDelete, key, nil}) return nil @@ -40,7 +40,7 @@ func (b *boltDBBatch) Delete(key []byte) error { // Write implements Batch. func (b *boltDBBatch) Write() error { if b.ops == nil { - return errBatchClosed + return ErrBatchClosed } err := b.db.db.Batch(func(tx *bbolt.Tx) error { bkt := tx.Bucket(bucket) diff --git a/cleveldb_batch.go b/cleveldb_batch.go index 85c6e0221..d776aae51 100644 --- a/cleveldb_batch.go +++ b/cleveldb_batch.go @@ -20,7 +20,7 @@ func newCLevelDBBatch(db *CLevelDB) *cLevelDBBatch { // Set implements Batch. func (b *cLevelDBBatch) Set(key, value []byte) error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } b.batch.Put(key, value) return nil @@ -29,7 +29,7 @@ func (b *cLevelDBBatch) Set(key, value []byte) error { // Delete implements Batch. func (b *cLevelDBBatch) Delete(key []byte) error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } b.batch.Delete(key) return nil @@ -38,7 +38,7 @@ func (b *cLevelDBBatch) Delete(key []byte) error { // Write implements Batch. func (b *cLevelDBBatch) Write() error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } err := b.db.db.Write(b.db.wo, b.batch) if err != nil { @@ -50,7 +50,7 @@ func (b *cLevelDBBatch) Write() error { // WriteSync implements Batch. func (b *cLevelDBBatch) WriteSync() error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } err := b.db.db.Write(b.db.woSync, b.batch) if err != nil { diff --git a/goleveldb_batch.go b/goleveldb_batch.go index aa66cb85c..dc65a47d3 100644 --- a/goleveldb_batch.go +++ b/goleveldb_batch.go @@ -22,7 +22,7 @@ func newGoLevelDBBatch(db *GoLevelDB) *goLevelDBBatch { // Set implements Batch. func (b *goLevelDBBatch) Set(key, value []byte) error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } b.batch.Put(key, value) return nil @@ -31,7 +31,7 @@ func (b *goLevelDBBatch) Set(key, value []byte) error { // Delete implements Batch. func (b *goLevelDBBatch) Delete(key []byte) error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } b.batch.Delete(key) return nil @@ -40,7 +40,7 @@ func (b *goLevelDBBatch) Delete(key []byte) error { // Write implements Batch. func (b *goLevelDBBatch) Write() error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: false}) if err != nil { @@ -52,7 +52,7 @@ func (b *goLevelDBBatch) Write() error { // WriteSync implements Batch. func (b *goLevelDBBatch) WriteSync() error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: true}) if err != nil { diff --git a/memdb_batch.go b/memdb_batch.go index 18b2baf83..9473d1454 100644 --- a/memdb_batch.go +++ b/memdb_batch.go @@ -35,7 +35,7 @@ func newMemDBBatch(db *MemDB) *memDBBatch { // Set implements Batch. func (b *memDBBatch) Set(key, value []byte) error { if b.ops == nil { - return errBatchClosed + return ErrBatchClosed } b.ops = append(b.ops, operation{opTypeSet, key, value}) return nil @@ -44,7 +44,7 @@ func (b *memDBBatch) Set(key, value []byte) error { // Delete implements Batch. func (b *memDBBatch) Delete(key []byte) error { if b.ops == nil { - return errBatchClosed + return ErrBatchClosed } b.ops = append(b.ops, operation{opTypeDelete, key, nil}) return nil @@ -53,7 +53,7 @@ func (b *memDBBatch) Delete(key []byte) error { // Write implements Batch. func (b *memDBBatch) Write() error { if b.ops == nil { - return errBatchClosed + return ErrBatchClosed } b.db.mtx.Lock() defer b.db.mtx.Unlock() diff --git a/rocksdb_batch.go b/rocksdb_batch.go index 73c662798..678b78506 100644 --- a/rocksdb_batch.go +++ b/rocksdb_batch.go @@ -21,7 +21,7 @@ func newRocksDBBatch(db *RocksDB) *rocksDBBatch { // Set implements Batch. func (b *rocksDBBatch) Set(key, value []byte) error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } b.batch.Put(key, value) return nil @@ -30,7 +30,7 @@ func (b *rocksDBBatch) Set(key, value []byte) error { // Delete implements Batch. func (b *rocksDBBatch) Delete(key []byte) error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } b.batch.Delete(key) return nil @@ -39,7 +39,7 @@ func (b *rocksDBBatch) Delete(key []byte) error { // Write implements Batch. func (b *rocksDBBatch) Write() error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } err := b.db.db.Write(b.db.wo, b.batch) if err != nil { @@ -51,7 +51,7 @@ func (b *rocksDBBatch) Write() error { // WriteSync mplements Batch. func (b *rocksDBBatch) WriteSync() error { if b.batch == nil { - return errBatchClosed + return ErrBatchClosed } err := b.db.db.Write(b.db.woSync, b.batch) if err != nil { diff --git a/types.go b/types.go index 5198a12a2..2cfc0e41b 100644 --- a/types.go +++ b/types.go @@ -3,7 +3,8 @@ package db import "github.com/pkg/errors" var ( - errBatchClosed = errors.New("batch is closed") + // ErrBatchClosed is returned for closed batches + ErrBatchClosed = errors.New("batch is closed") ) // DBs are goroutine safe. From f579de87168067945e3eb4683459c289cc699cb2 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:49:08 +0100 Subject: [PATCH 11/17] Ported RemoteDB to new batch interface --- remotedb/batch.go | 23 +++++++++++++++++++++-- remotedb/remotedb.go | 5 +---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/remotedb/batch.go b/remotedb/batch.go index c93cf2e59..0326b1ba5 100644 --- a/remotedb/batch.go +++ b/remotedb/batch.go @@ -14,8 +14,18 @@ type batch struct { var _ db.Batch = (*batch)(nil) +func newBatch(rdb *RemoteDB) *batch { + return &batch{ + db: rdb, + ops: make([]*protodb.Operation, 0, 1), + } +} + // Set implements Batch. func (b *batch) Set(key, value []byte) error { + if b.ops == nil { + return db.ErrBatchClosed + } op := &protodb.Operation{ Entity: &protodb.Entity{Key: key, Value: value}, Type: protodb.Operation_SET, @@ -26,6 +36,9 @@ func (b *batch) Set(key, value []byte) error { // Delete implements Batch. func (b *batch) Delete(key []byte) error { + if b.ops == nil { + return db.ErrBatchClosed + } op := &protodb.Operation{ Entity: &protodb.Entity{Key: key}, Type: protodb.Operation_DELETE, @@ -36,18 +49,24 @@ func (b *batch) Delete(key []byte) error { // Write implements Batch. func (b *batch) Write() error { + if b.ops == nil { + return db.ErrBatchClosed + } if _, err := b.db.dc.BatchWrite(b.db.ctx, &protodb.Batch{Ops: b.ops}); err != nil { return errors.Errorf("remoteDB.BatchWrite: %v", err) } - return nil + return b.Close() } // WriteSync implements Batch. func (b *batch) WriteSync() error { + if b.ops == nil { + return db.ErrBatchClosed + } if _, err := b.db.dc.BatchWriteSync(b.db.ctx, &protodb.Batch{Ops: b.ops}); err != nil { return errors.Errorf("RemoteDB.BatchWriteSync: %v", err) } - return nil + return b.Close() } // Close implements Batch. diff --git a/remotedb/remotedb.go b/remotedb/remotedb.go index 9da3dd08b..02822807c 100644 --- a/remotedb/remotedb.go +++ b/remotedb/remotedb.go @@ -98,10 +98,7 @@ func (rd *RemoteDB) ReverseIterator(start, end []byte) (db.Iterator, error) { } func (rd *RemoteDB) NewBatch() db.Batch { - return &batch{ - db: rd, - ops: nil, - } + return newBatch(rd) } // TODO: Implement Print when db.DB implements a method From 50bea1534259c27feb91f5580ee48bad784b751d Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:53:16 +0100 Subject: [PATCH 12/17] Changelog clarification --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54ffcee90..ec64ae061 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ - `Batch` interface methods now return errors -- Closed batches can no longer be reused +- Closed batches can no longer be reused, all non-`Close()` calls return `ErrBatchClosed` - Removed the `SetDeleter` interface From 1b5ff54e4e68813a42f92ac71db122db51368d71 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 10:54:05 +0100 Subject: [PATCH 13/17] Added changelog entry for implicit batch closing --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec64ae061..30de7a905 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ - Closed batches can no longer be reused, all non-`Close()` calls return `ErrBatchClosed` +- Calling `Write()` or `WriteSync()` on a `Batch` will implicitly close it + - Removed the `SetDeleter` interface - [memdb] [\#56](https://github.com/tendermint/tm-db/pull/56) Removed some exported methods that were mainly meant for internal use: `Mutex()`, `SetNoLock()`, `SetNoLockSync()`, `DeleteNoLock()`, and `DeleteNoLockSync()` From 518db8037bfe6a2ce7f7b47f5fb98460edb35702 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 11:49:23 +0100 Subject: [PATCH 14/17] Test workaround for levigo bug --- backend_test.go | 29 ++++++++++++++++------------- cleveldb_batch.go | 4 ++-- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/backend_test.go b/backend_test.go index 4ee6d6617..ebda566c8 100644 --- a/backend_test.go +++ b/backend_test.go @@ -456,19 +456,22 @@ func testDBBatch(t *testing.T, backend BackendType) { assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}}) // writing nil keys and values should be the same as empty keys and values - batch = db.NewBatch() - 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() - 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}}) + // FIXME CLevelDB panics here: https://github.com/jmhodges/levigo/issues/55 + if backend != CLevelDBBackend { + batch = db.NewBatch() + 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() + 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}}) + } // it should be possible to write an empty batch batch = db.NewBatch() diff --git a/cleveldb_batch.go b/cleveldb_batch.go index d776aae51..66729c0ab 100644 --- a/cleveldb_batch.go +++ b/cleveldb_batch.go @@ -22,7 +22,7 @@ func (b *cLevelDBBatch) Set(key, value []byte) error { if b.batch == nil { return ErrBatchClosed } - b.batch.Put(key, value) + b.batch.Put(nonNilBytes(key), nonNilBytes(value)) return nil } @@ -31,7 +31,7 @@ func (b *cLevelDBBatch) Delete(key []byte) error { if b.batch == nil { return ErrBatchClosed } - b.batch.Delete(key) + b.batch.Delete(nonNilBytes(key)) return nil } From 131b3e7018c1833bf5746aee6523ec2e36baacfb Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 13:53:22 +0100 Subject: [PATCH 15/17] Panic on use of closed or written batch --- CHANGELOG.md | 12 +++------- backend_test.go | 56 +++++++++++++++++++++++----------------------- boltdb_batch.go | 29 ++++++++++++------------ cleveldb_batch.go | 37 +++++++++++++++--------------- goleveldb_batch.go | 44 +++++++++++++++++------------------- memdb_batch.go | 30 ++++++++++++------------- prefixdb_batch.go | 12 +++++----- remotedb/batch.go | 43 ++++++++++++++++++----------------- rocksdb_batch.go | 39 ++++++++++++++++---------------- types.go | 35 ++++++++++++++--------------- 10 files changed, 162 insertions(+), 175 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30de7a905..7e4eb430c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,13 +4,7 @@ ### Breaking Changes -- `Batch` interface methods now return errors - -- Closed batches can no longer be reused, all non-`Close()` calls return `ErrBatchClosed` - -- Calling `Write()` or `WriteSync()` on a `Batch` will implicitly close it - -- Removed the `SetDeleter` interface +- [\#71](https://github.com/tendermint/tm-db/pull/71) Closed or written batches can no longer be reused, all non-`Close()` calls will panic - [memdb] [\#56](https://github.com/tendermint/tm-db/pull/56) Removed some exported methods that were mainly meant for internal use: `Mutex()`, `SetNoLock()`, `SetNoLockSync()`, `DeleteNoLock()`, and `DeleteNoLockSync()` @@ -22,9 +16,9 @@ ### Bug Fixes -- [boltdb] Properly handle blank keys in iterators +- [boltdb] [\#69](https://github.com/tendermint/tm-db/pull/69) Properly handle blank keys in iterators -- [cleveldb] Fix handling of empty keys as iterator endpoints +- [cleveldb] [\#65](https://github.com/tendermint/tm-db/pull/65) Fix handling of empty keys as iterator endpoints ## 0.4.1 diff --git a/backend_test.go b/backend_test.go index ebda566c8..47fedaa04 100644 --- a/backend_test.go +++ b/backend_test.go @@ -430,44 +430,46 @@ 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() - 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})) + batch.Set([]byte("a"), []byte{1}) + batch.Set([]byte("b"), []byte{2}) + 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}}) - // writing a batch should implicitly close it, so writing it again should error - err = batch.WriteSync() - require.Equal(t, ErrBatchClosed, err) + // 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() }) + require.Panics(t, func() { batch.WriteSync() }) + batch.Close() // batches should write changes in order batch = db.NewBatch() - 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()) + 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() 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() - err = batch.Set(nil, nil) - require.NoError(t, err) + batch.Set(nil, nil) err = batch.WriteSync() require.NoError(t, err) assertKeyValues(t, db, map[string][]byte{"": {}, "a": {1}, "b": {2}}) batch = db.NewBatch() - err = batch.Delete(nil) - require.NoError(t, err) + batch.Delete(nil) err = batch.Write() require.NoError(t, err) assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}}) @@ -481,16 +483,14 @@ func testDBBatch(t *testing.T, backend BackendType) { // it should be possible to close an empty batch, and to re-close a closed batch batch = db.NewBatch() - err = batch.Close() - require.NoError(t, err) - err = batch.Close() - require.NoError(t, err) - - // all other operations on a closed batch should error - require.Equal(t, ErrBatchClosed, batch.Set([]byte("a"), []byte{9})) - require.Equal(t, ErrBatchClosed, batch.Delete([]byte("a"))) - require.Equal(t, ErrBatchClosed, batch.Write()) - require.Equal(t, ErrBatchClosed, batch.WriteSync()) + 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() }) + require.Panics(t, func() { batch.WriteSync() }) } func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) { diff --git a/boltdb_batch.go b/boltdb_batch.go index cb8ed791b..d6d3cc125 100644 --- a/boltdb_batch.go +++ b/boltdb_batch.go @@ -19,29 +19,27 @@ func newBoltDBBatch(db *BoltDB) *boltDBBatch { } } -// Set implements Batch. -func (b *boltDBBatch) Set(key, value []byte) error { +func (b *boltDBBatch) assertOpen() { if b.ops == nil { - return ErrBatchClosed + panic("batch has been written or closed") } +} + +// 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) error { - if b.ops == nil { - return ErrBatchClosed - } +func (b *boltDBBatch) Delete(key []byte) { + b.assertOpen() b.ops = append(b.ops, operation{opTypeDelete, key, nil}) - return nil } // Write implements Batch. func (b *boltDBBatch) Write() error { - if b.ops == nil { - return ErrBatchClosed - } + b.assertOpen() err := b.db.db.Batch(func(tx *bbolt.Tx) error { bkt := tx.Bucket(bucket) for _, op := range b.ops { @@ -62,7 +60,9 @@ func (b *boltDBBatch) Write() error { if err != nil { return err } - return b.Close() + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // WriteSync implements Batch. @@ -71,7 +71,6 @@ func (b *boltDBBatch) WriteSync() error { } // Close implements Batch. -func (b *boltDBBatch) Close() error { +func (b *boltDBBatch) Close() { b.ops = nil - return nil } diff --git a/cleveldb_batch.go b/cleveldb_batch.go index 66729c0ab..eeb2770d2 100644 --- a/cleveldb_batch.go +++ b/cleveldb_batch.go @@ -17,53 +17,52 @@ func newCLevelDBBatch(db *CLevelDB) *cLevelDBBatch { } } -// Set implements Batch. -func (b *cLevelDBBatch) Set(key, value []byte) error { +func (b *cLevelDBBatch) assertOpen() { if b.batch == nil { - return ErrBatchClosed + panic("batch has been written or closed") } +} + +// 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) error { - if b.batch == nil { - return ErrBatchClosed - } +func (b *cLevelDBBatch) Delete(key []byte) { + b.assertOpen() b.batch.Delete(nonNilBytes(key)) - return nil } // Write implements Batch. func (b *cLevelDBBatch) Write() error { - if b.batch == nil { - return ErrBatchClosed - } + b.assertOpen() err := b.db.db.Write(b.db.wo, b.batch) if err != nil { return err } - return b.Close() + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // WriteSync implements Batch. func (b *cLevelDBBatch) WriteSync() error { - if b.batch == nil { - return ErrBatchClosed - } + b.assertOpen() err := b.db.db.Write(b.db.woSync, b.batch) if err != nil { return err } - return b.Close() + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // Close implements Batch. -func (b *cLevelDBBatch) Close() error { +func (b *cLevelDBBatch) Close() { if b.batch != nil { b.batch.Close() b.batch = nil } - return nil } diff --git a/goleveldb_batch.go b/goleveldb_batch.go index dc65a47d3..efb33162a 100644 --- a/goleveldb_batch.go +++ b/goleveldb_batch.go @@ -19,53 +19,49 @@ func newGoLevelDBBatch(db *GoLevelDB) *goLevelDBBatch { } } -// Set implements Batch. -func (b *goLevelDBBatch) Set(key, value []byte) error { +func (b *goLevelDBBatch) assertOpen() { if b.batch == nil { - return ErrBatchClosed + panic("batch has been written or closed") } +} + +// 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) error { - if b.batch == nil { - return ErrBatchClosed - } +func (b *goLevelDBBatch) Delete(key []byte) { + b.assertOpen() b.batch.Delete(key) - return nil } // Write implements Batch. func (b *goLevelDBBatch) Write() error { - if b.batch == nil { - return ErrBatchClosed - } - err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: false}) - if err != nil { - return err - } - return b.Close() + return b.write(false) } // WriteSync implements Batch. func (b *goLevelDBBatch) WriteSync() error { - if b.batch == nil { - return ErrBatchClosed - } - err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: true}) + return b.write(true) +} + +func (b *goLevelDBBatch) write(sync bool) error { + b.assertOpen() + err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: sync}) if err != nil { return err } - return b.Close() + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // Close implements Batch. -func (b *goLevelDBBatch) Close() error { +func (b *goLevelDBBatch) Close() { if b.batch != nil { b.batch.Reset() b.batch = nil } - return nil } diff --git a/memdb_batch.go b/memdb_batch.go index 9473d1454..1312fb0de 100644 --- a/memdb_batch.go +++ b/memdb_batch.go @@ -32,29 +32,27 @@ func newMemDBBatch(db *MemDB) *memDBBatch { } } -// Set implements Batch. -func (b *memDBBatch) Set(key, value []byte) error { +func (b *memDBBatch) assertOpen() { if b.ops == nil { - return ErrBatchClosed + panic("batch has been written or closed") } +} + +// 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) error { - if b.ops == nil { - return ErrBatchClosed - } +func (b *memDBBatch) Delete(key []byte) { + b.assertOpen() b.ops = append(b.ops, operation{opTypeDelete, key, nil}) - return nil } // Write implements Batch. func (b *memDBBatch) Write() error { - if b.ops == nil { - return ErrBatchClosed - } + b.assertOpen() b.db.mtx.Lock() defer b.db.mtx.Unlock() @@ -68,7 +66,10 @@ func (b *memDBBatch) Write() error { return errors.Errorf("unknown operation type %v (%v)", op.opType, op) } } - return b.Close() + + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // WriteSync implements Batch. @@ -77,7 +78,6 @@ func (b *memDBBatch) WriteSync() error { } // Close implements Batch. -func (b *memDBBatch) Close() error { +func (b *memDBBatch) Close() { b.ops = nil - return nil } diff --git a/prefixdb_batch.go b/prefixdb_batch.go index 73433c2ee..a3547de18 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) error { +func (pb prefixDBBatch) Set(key, value []byte) { pkey := append(cp(pb.prefix), key...) - return pb.source.Set(pkey, value) + pb.source.Set(pkey, value) } // Delete implements Batch. -func (pb prefixDBBatch) Delete(key []byte) error { +func (pb prefixDBBatch) Delete(key []byte) { pkey := append(cp(pb.prefix), key...) - return pb.source.Delete(pkey) + pb.source.Delete(pkey) } // Write implements Batch. @@ -37,6 +37,6 @@ func (pb prefixDBBatch) WriteSync() error { } // Close implements Batch. -func (pb prefixDBBatch) Close() error { - return pb.source.Close() +func (pb prefixDBBatch) Close() { + pb.source.Close() } diff --git a/remotedb/batch.go b/remotedb/batch.go index 0326b1ba5..6970f07e0 100644 --- a/remotedb/batch.go +++ b/remotedb/batch.go @@ -21,56 +21,57 @@ func newBatch(rdb *RemoteDB) *batch { } } -// Set implements Batch. -func (b *batch) Set(key, value []byte) error { +func (b *batch) assertOpen() { if b.ops == nil { - return db.ErrBatchClosed + panic("batch has been written or closed") } +} + +// 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) error { - if b.ops == nil { - return db.ErrBatchClosed - } +func (b *batch) Delete(key []byte) { + b.assertOpen() 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 { - if b.ops == nil { - return db.ErrBatchClosed - } - if _, err := b.db.dc.BatchWrite(b.db.ctx, &protodb.Batch{Ops: b.ops}); err != nil { + b.assertOpen() + _, err := b.db.dc.BatchWrite(b.db.ctx, &protodb.Batch{Ops: b.ops}) + if err != nil { return errors.Errorf("remoteDB.BatchWrite: %v", err) } - return b.Close() + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // WriteSync implements Batch. func (b *batch) WriteSync() error { - if b.ops == nil { - return db.ErrBatchClosed - } - if _, err := b.db.dc.BatchWriteSync(b.db.ctx, &protodb.Batch{Ops: b.ops}); err != nil { + b.assertOpen() + _, err := b.db.dc.BatchWriteSync(b.db.ctx, &protodb.Batch{Ops: b.ops}) + if err != nil { return errors.Errorf("RemoteDB.BatchWriteSync: %v", err) } - return b.Close() + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // Close implements Batch. -func (b *batch) Close() error { +func (b *batch) Close() { b.ops = nil - return nil } diff --git a/rocksdb_batch.go b/rocksdb_batch.go index 678b78506..9cab3df6d 100644 --- a/rocksdb_batch.go +++ b/rocksdb_batch.go @@ -18,53 +18,52 @@ func newRocksDBBatch(db *RocksDB) *rocksDBBatch { } } -// Set implements Batch. -func (b *rocksDBBatch) Set(key, value []byte) error { +func (b *rocksDBBatch) assertOpen() { if b.batch == nil { - return ErrBatchClosed + panic("batch has been written or closed") } +} + +// 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) error { - if b.batch == nil { - return ErrBatchClosed - } +func (b *rocksDBBatch) Delete(key []byte) { + b.assertOpen() b.batch.Delete(key) - return nil } // Write implements Batch. func (b *rocksDBBatch) Write() error { - if b.batch == nil { - return ErrBatchClosed - } + b.assertOpen() err := b.db.db.Write(b.db.wo, b.batch) if err != nil { return err } - return b.Close() + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } -// WriteSync mplements Batch. +// WriteSync implements Batch. func (b *rocksDBBatch) WriteSync() error { - if b.batch == nil { - return ErrBatchClosed - } + b.assertOpen() err := b.db.db.Write(b.db.woSync, b.batch) if err != nil { return err } - return b.Close() + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // Close implements Batch. -func (b *rocksDBBatch) Close() error { +func (b *rocksDBBatch) Close() { if b.batch != nil { b.batch.Destroy() b.batch = nil } - return nil } diff --git a/types.go b/types.go index 2cfc0e41b..ad1dc587a 100644 --- a/types.go +++ b/types.go @@ -1,12 +1,5 @@ package db -import "github.com/pkg/errors" - -var ( - // ErrBatchClosed is returned for closed batches - ErrBatchClosed = errors.New("batch is closed") -) - // DBs are goroutine safe. type DB interface { @@ -63,22 +56,28 @@ type DB interface { // Batch Close must be called when the program no longer needs the object. type Batch interface { - // Set sets a key/value pair. - // CONTRACT: key, value readonly []byte - Set(key, value []byte) error + SetDeleter - // Delete deles a key. - // CONTRACT: key readonly []byte - Delete(key []byte) error - - // Write writes the batch, possibly without flushing to disk. It implicitly closes the batch. + // Write writes the batch, possibly without flushing to disk. Only Close() can be called after, + // other methods will panic. Write() error - // WriteSync writes the batch and flushes it to disk. It implicitly closes the batch. + // WriteSync writes the batch and flushes it to disk. Only Close() can be called after, other + // methods will panic. WriteSync() error - // Close closes the batch. It is idempotent, but the batch cannot be used afterwards. - Close() 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) } /* From ebf819f686584663d9678c7c4235a477a40d8f90 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 13:57:42 +0100 Subject: [PATCH 16/17] Disable errcheck lint in tests --- backend_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/backend_test.go b/backend_test.go index 47fedaa04..9550fc3dc 100644 --- a/backend_test.go +++ b/backend_test.go @@ -442,8 +442,8 @@ func testDBBatch(t *testing.T, backend BackendType) { // 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() }) - require.Panics(t, func() { batch.WriteSync() }) + require.Panics(t, func() { batch.Write() }) // nolint: errcheck + require.Panics(t, func() { batch.WriteSync() }) // nolint: errcheck batch.Close() // batches should write changes in order @@ -489,8 +489,8 @@ func testDBBatch(t *testing.T, backend BackendType) { // 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() }) - require.Panics(t, func() { batch.WriteSync() }) + require.Panics(t, func() { batch.Write() }) // nolint: errcheck + require.Panics(t, func() { batch.WriteSync() }) // nolint: errcheck } func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) { From 76d78b60feadaa9c27cf9a79eae6d721f9fe0573 Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Tue, 10 Mar 2020 15:52:05 +0100 Subject: [PATCH 17/17] Don't set initial slice capacities --- boltdb_batch.go | 2 +- memdb_batch.go | 2 +- remotedb/batch.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/boltdb_batch.go b/boltdb_batch.go index d6d3cc125..f3eaf9ce3 100644 --- a/boltdb_batch.go +++ b/boltdb_batch.go @@ -15,7 +15,7 @@ var _ Batch = (*boltDBBatch)(nil) func newBoltDBBatch(db *BoltDB) *boltDBBatch { return &boltDBBatch{ db: db, - ops: make([]operation, 0, 1), + ops: []operation{}, } } diff --git a/memdb_batch.go b/memdb_batch.go index 1312fb0de..d9b94999c 100644 --- a/memdb_batch.go +++ b/memdb_batch.go @@ -28,7 +28,7 @@ var _ Batch = (*memDBBatch)(nil) func newMemDBBatch(db *MemDB) *memDBBatch { return &memDBBatch{ db: db, - ops: make([]operation, 0, 1), + ops: []operation{}, } } diff --git a/remotedb/batch.go b/remotedb/batch.go index 6970f07e0..8187fd7f3 100644 --- a/remotedb/batch.go +++ b/remotedb/batch.go @@ -17,7 +17,7 @@ var _ db.Batch = (*batch)(nil) func newBatch(rdb *RemoteDB) *batch { return &batch{ db: rdb, - ops: make([]*protodb.Operation, 0, 1), + ops: []*protodb.Operation{}, } }