diff --git a/CHANGELOG.md b/CHANGELOG.md index d89f6e047..7e4eb430c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Breaking Changes +- [\#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()` ### Improvements @@ -14,11 +16,9 @@ ### Bug Fixes -- [boltdb] Properly handle blank keys in iterators - -- [cleveldb] Fix handling of empty keys as iterator endpoints +- [boltdb] [\#69](https://github.com/tendermint/tm-db/pull/69) Properly handle blank keys in iterators -- [goleveldb] [\#58](https://github.com/tendermint/tm-db/pull/58) Make `Batch.Close()` actually remove the batch contents +- [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 423d7ba77..9550fc3dc 100644 --- a/backend_test.go +++ b/backend_test.go @@ -439,41 +439,14 @@ func testDBBatch(t *testing.T, backend BackendType) { 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) - err = batch.WriteSync() - require.NoError(t, err) - assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}, "c": {3}}) - - // 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) - 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() - 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}}) + // 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() - // batches should also write changes in order + // batches should write changes in order batch = db.NewBatch() batch.Delete([]byte("a")) batch.Set([]byte("a"), []byte{1}) @@ -486,13 +459,38 @@ func testDBBatch(t *testing.T, backend BackendType) { batch.Close() assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}}) - // and writing an empty batch should not fail + // 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.WriteSync() + require.NoError(t, err) + assertKeyValues(t, db, map[string][]byte{"": {}, "a": {1}, "b": {2}}) + + batch = db.NewBatch() + batch.Delete(nil) + 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() 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() + 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 } func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) { 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 a5996fe38..f3eaf9ce3 100644 --- a/boltdb_batch.go +++ b/boltdb_batch.go @@ -12,19 +12,35 @@ type boltDBBatch struct { var _ Batch = (*boltDBBatch)(nil) +func newBoltDBBatch(db *BoltDB) *boltDBBatch { + return &boltDBBatch{ + db: db, + ops: []operation{}, + } +} + +func (b *boltDBBatch) assertOpen() { + if b.ops == nil { + 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}) } // Delete implements Batch. func (b *boltDBBatch) Delete(key []byte) { + b.assertOpen() b.ops = append(b.ops, operation{opTypeDelete, key, nil}) } // Write implements Batch. func (b *boltDBBatch) Write() error { - return b.db.db.Batch(func(tx *bbolt.Tx) error { + b.assertOpen() + err := b.db.db.Batch(func(tx *bbolt.Tx) error { bkt := tx.Bucket(bucket) for _, op := range b.ops { key := nonEmptyKey(nonNilBytes(op.key)) @@ -41,6 +57,12 @@ func (b *boltDBBatch) Write() error { } return nil }) + if err != nil { + return err + } + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() + return nil } // WriteSync implements Batch. @@ -49,4 +71,6 @@ func (b *boltDBBatch) WriteSync() error { } // Close implements Batch. -func (b *boltDBBatch) Close() {} +func (b *boltDBBatch) Close() { + b.ops = nil +} 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 c98fe3354..eeb2770d2 100644 --- a/cleveldb_batch.go +++ b/cleveldb_batch.go @@ -10,33 +10,59 @@ type cLevelDBBatch struct { batch *levigo.WriteBatch } +func newCLevelDBBatch(db *CLevelDB) *cLevelDBBatch { + return &cLevelDBBatch{ + db: db, + batch: levigo.NewWriteBatch(), + } +} + +func (b *cLevelDBBatch) assertOpen() { + if b.batch == nil { + panic("batch has been written or closed") + } +} + // Set implements Batch. func (b *cLevelDBBatch) Set(key, value []byte) { - b.batch.Put(key, value) + b.assertOpen() + b.batch.Put(nonNilBytes(key), nonNilBytes(value)) } // Delete implements Batch. func (b *cLevelDBBatch) Delete(key []byte) { - b.batch.Delete(key) + b.assertOpen() + b.batch.Delete(nonNilBytes(key)) } // Write implements Batch. func (b *cLevelDBBatch) Write() error { - if err := b.db.db.Write(b.db.wo, b.batch); err != nil { + b.assertOpen() + 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 } // WriteSync implements Batch. func (b *cLevelDBBatch) WriteSync() error { - if err := b.db.db.Write(b.db.woSync, b.batch); err != nil { + b.assertOpen() + 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 } // Close implements Batch. func (b *cLevelDBBatch) Close() { - b.batch.Close() + if b.batch != nil { + b.batch.Close() + b.batch = nil + } } 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 ec290fe10..efb33162a 100644 --- a/goleveldb_batch.go +++ b/goleveldb_batch.go @@ -12,35 +12,56 @@ type goLevelDBBatch struct { var _ Batch = (*goLevelDBBatch)(nil) +func newGoLevelDBBatch(db *GoLevelDB) *goLevelDBBatch { + return &goLevelDBBatch{ + db: db, + batch: new(leveldb.Batch), + } +} + +func (b *goLevelDBBatch) assertOpen() { + if b.batch == nil { + panic("batch has been written or closed") + } +} + // Set implements Batch. func (b *goLevelDBBatch) Set(key, value []byte) { + b.assertOpen() b.batch.Put(key, value) } // Delete implements Batch. func (b *goLevelDBBatch) Delete(key []byte) { + b.assertOpen() b.batch.Delete(key) } // Write implements Batch. func (b *goLevelDBBatch) Write() error { - err := b.db.db.Write(b.batch, &opt.WriteOptions{Sync: false}) - if err != nil { - return err - } - return nil + return b.write(false) } // WriteSync implements Batch. func (b *goLevelDBBatch) WriteSync() error { - 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 } + // 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() { - b.batch.Reset() + if b.batch != nil { + b.batch.Reset() + b.batch = nil + } } 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 4bd1f1d0c..d9b94999c 100644 --- a/memdb_batch.go +++ b/memdb_batch.go @@ -24,18 +24,35 @@ type memDBBatch struct { var _ Batch = (*memDBBatch)(nil) +// newMemDBBatch creates a new memDBBatch +func newMemDBBatch(db *MemDB) *memDBBatch { + return &memDBBatch{ + db: db, + ops: []operation{}, + } +} + +func (b *memDBBatch) assertOpen() { + if b.ops == nil { + 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}) } // Delete implements Batch. func (b *memDBBatch) Delete(key []byte) { + b.assertOpen() b.ops = append(b.ops, operation{opTypeDelete, key, nil}) } // Write implements Batch. func (b *memDBBatch) Write() error { + b.assertOpen() b.db.mtx.Lock() defer b.db.mtx.Unlock() @@ -49,6 +66,9 @@ func (b *memDBBatch) Write() error { return errors.Errorf("unknown operation type %v (%v)", op.opType, op) } } + + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() return nil } diff --git a/remotedb/batch.go b/remotedb/batch.go index 5fb92f30c..8187fd7f3 100644 --- a/remotedb/batch.go +++ b/remotedb/batch.go @@ -14,8 +14,22 @@ type batch struct { var _ db.Batch = (*batch)(nil) +func newBatch(rdb *RemoteDB) *batch { + return &batch{ + db: rdb, + ops: []*protodb.Operation{}, + } +} + +func (b *batch) assertOpen() { + if b.ops == nil { + 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, @@ -25,6 +39,7 @@ func (b *batch) Set(key, value []byte) { // Delete implements Batch. func (b *batch) Delete(key []byte) { + b.assertOpen() op := &protodb.Operation{ Entity: &protodb.Entity{Key: key}, Type: protodb.Operation_DELETE, @@ -34,17 +49,25 @@ func (b *batch) Delete(key []byte) { // Write implements Batch. func (b *batch) Write() error { - 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) } + // 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 _, 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) } + // Make sure batch cannot be used afterwards. Callers should still call Close(), for errors. + b.Close() return nil } 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 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 085ec51ce..9cab3df6d 100644 --- a/rocksdb_batch.go +++ b/rocksdb_batch.go @@ -11,35 +11,59 @@ type rocksDBBatch struct { var _ Batch = (*rocksDBBatch)(nil) +func newRocksDBBatch(db *RocksDB) *rocksDBBatch { + return &rocksDBBatch{ + db: db, + batch: gorocksdb.NewWriteBatch(), + } +} + +func (b *rocksDBBatch) assertOpen() { + if b.batch == nil { + panic("batch has been written or closed") + } +} + // Set implements Batch. -func (mBatch *rocksDBBatch) Set(key, value []byte) { - mBatch.batch.Put(key, value) +func (b *rocksDBBatch) Set(key, value []byte) { + b.assertOpen() + b.batch.Put(key, value) } // Delete implements Batch. -func (mBatch *rocksDBBatch) Delete(key []byte) { - mBatch.batch.Delete(key) +func (b *rocksDBBatch) Delete(key []byte) { + b.assertOpen() + b.batch.Delete(key) } // Write implements Batch. -func (mBatch *rocksDBBatch) Write() error { - err := mBatch.db.db.Write(mBatch.db.wo, mBatch.batch) +func (b *rocksDBBatch) Write() error { + b.assertOpen() + 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 } -// WriteSync mplements Batch. -func (mBatch *rocksDBBatch) WriteSync() error { - err := mBatch.db.db.Write(mBatch.db.woSync, mBatch.batch) +// WriteSync implements Batch. +func (b *rocksDBBatch) WriteSync() error { + b.assertOpen() + 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 } // Close implements Batch. -func (mBatch *rocksDBBatch) Close() { - mBatch.batch.Destroy() +func (b *rocksDBBatch) Close() { + if b.batch != nil { + b.batch.Destroy() + b.batch = nil + } } diff --git a/types.go b/types.go index 1de83e96f..ad1dc587a 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,24 +54,31 @@ 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 + + // 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. Only Close() can be called after, other + // methods will panic. WriteSync() error + + // Close closes the batch. It is idempotent, but any other calls afterwards will panic. Close() } type SetDeleter interface { - Set(key, value []byte) // CONTRACT: key, value readonly []byte - Delete(key []byte) // CONTRACT: key readonly []byte -} + // Set sets a key/value pair. + // CONTRACT: key, value readonly []byte + Set(key, value []byte) -//---------------------------------------- -// Iterator + // Delete deletes a key/value pair. + // CONTRACT: key readonly []byte + Delete(key []byte) +} /* Usage: