Calling non-Close() methods on closed or written batches panic

CHANGELOG.md

@@ -4,6 +4,14 @@
 
 ### 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
+
 - [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
@@ -18,8 +26,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**

backend_test.go

@@ -430,69 +430,67 @@ 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()
-	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()
+	// 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)
+		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}})
+	}
 
-	// batches should also write changes in order
+	// it should be possible to write an empty batch
 	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()
 	assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}})
 
-	// and writing an empty batch should not fail
+	// it should be possible to close an empty batch, and to re-close a closed batch
 	batch = db.NewBatch()
-	err = batch.Write()
+	err = batch.Close()
 	require.NoError(t, err)
-	err = batch.WriteSync()
+	err = batch.Close()
 	require.NoError(t, err)
-	assertKeyValues(t, db, map[string][]byte{"a": {1}, "b": {2}})
+
+	// 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) {

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

boltdb_batch.go

@@ -12,19 +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) {
+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) {
+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))
@@ -41,6 +59,10 @@ func (b *boltDBBatch) Write() error {
 		}
 		return nil
 	})
+	if err != nil {
+		return err
+	}
+	return b.Close()
 }
 
 // WriteSync implements Batch.
@@ -49,4 +71,7 @@ func (b *boltDBBatch) WriteSync() error {
 }
 
 // Close implements Batch.
-func (b *boltDBBatch) Close() {}
+func (b *boltDBBatch) Close() error {
+	b.ops = nil
+	return nil
+}

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.

cleveldb_batch.go

@@ -10,33 +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) {
-	b.batch.Put(key, value)
+func (b *cLevelDBBatch) Set(key, value []byte) error {
+	if b.batch == nil {
+		return ErrBatchClosed
+	}
+	b.batch.Put(nonNilBytes(key), nonNilBytes(value))
+	return nil
 }
 
 // Delete implements Batch.
-func (b *cLevelDBBatch) Delete(key []byte) {
-	b.batch.Delete(key)
+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 {
-	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() {
-	b.batch.Close()
+func (b *cLevelDBBatch) Close() error {
+	if b.batch != nil {
+		b.batch.Close()
+		b.batch = nil
+	}
+	return nil
 }

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.

goleveldb_batch.go

@@ -12,35 +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) {
+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) {
+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() {
-	b.batch.Reset()
+func (b *goLevelDBBatch) Close() error {
+	if b.batch != nil {
+		b.batch.Reset()
+		b.batch = nil
+	}
+	return nil
 }

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.