iterator: improve and document error handling (#97)

The iterator pattern we're using typically panics on use of invalid iterators (including our backends). I've kept this pattern, but tried to reduce the amount of panics and return via `Error()` instead, and also made `Close()` panic.

Our iterator examples did not include checking `Error()`, so this is likely to break many applications that today rely on iterator panics for error handling (e.g. IAVL, see cosmos/iavl#247).

CHANGELOG.md

@@ -6,6 +6,13 @@
 
 - Batch `Set()`, `Delete()`, and `Close()` may now error (@erikgrinaker)
 
+- `Iterator.Close()` may now error (@erikgrinaker)
+
+- Many iterator panics are now exposed via `Error()` instead (@erikgrinaker)
+
+- `RemoteDB` iterators are now correctly primed with the first item when created, without calling
+  `Next()` (@erikgrinaker)
+
 - The `SetDeleter` interface has been removed (@erikgrinaker)
 
 ## 0.5.1

boltdb_iterator.go

@@ -95,6 +95,11 @@ func (itr *boltDBIterator) Valid() bool {
 		return false
 	}
 
+	if itr.Error() != nil {
+		itr.isInvalid = true
+		return false
+	}
+
 	// iterated to the end of the cursor
 	if itr.currentKey == nil {
 		itr.isInvalid = true
@@ -165,15 +170,12 @@ func (itr *boltDBIterator) Error() error {
 }
 
 // Close implements Iterator.
-func (itr *boltDBIterator) Close() {
-	err := itr.tx.Rollback()
-	if err != nil {
-		panic(err)
-	}
+func (itr *boltDBIterator) Close() error {
+	return itr.tx.Rollback()
 }
 
 func (itr *boltDBIterator) assertIsValid() {
 	if !itr.Valid() {
-		panic("boltdb-iterator is invalid")
+		panic("iterator is invalid")
 	}
 }

cleveldb_iterator.go

@@ -62,8 +62,11 @@ func (itr cLevelDBIterator) Valid() bool {
 		return false
 	}
 
-	// Panic on DB error.  No way to recover.
-	itr.assertNoError()
+	// If source errors, invalid.
+	if itr.source.GetError() != nil {
+		itr.isInvalid = true
+		return false
+	}
 
 	// If source is invalid, invalid.
 	if !itr.source.Valid() {
@@ -93,21 +96,18 @@ func (itr cLevelDBIterator) Valid() bool {
 
 // Key implements Iterator.
 func (itr cLevelDBIterator) Key() []byte {
-	itr.assertNoError()
 	itr.assertIsValid()
 	return itr.source.Key()
 }
 
 // Value implements Iterator.
 func (itr cLevelDBIterator) Value() []byte {
-	itr.assertNoError()
 	itr.assertIsValid()
 	return itr.source.Value()
 }
 
 // Next implements Iterator.
 func (itr cLevelDBIterator) Next() {
-	itr.assertNoError()
 	itr.assertIsValid()
 	if itr.isReverse {
 		itr.source.Prev()
@@ -122,19 +122,13 @@ func (itr cLevelDBIterator) Error() error {
 }
 
 // Close implements Iterator.
-func (itr cLevelDBIterator) Close() {
+func (itr cLevelDBIterator) Close() error {
 	itr.source.Close()
-}
-
-func (itr cLevelDBIterator) assertNoError() {
-	err := itr.source.GetError()
-	if err != nil {
-		panic(err)
-	}
+	return nil
 }
 
 func (itr cLevelDBIterator) assertIsValid() {
 	if !itr.Valid() {
-		panic("cLevelDBIterator is invalid")
+		panic("iterator is invalid")
 	}
 }

goleveldb_iterator.go

@@ -60,8 +60,11 @@ func (itr *goLevelDBIterator) Valid() bool {
 		return false
 	}
 
-	// Panic on DB error.  No way to recover.
-	itr.assertNoError()
+	// If source errors, invalid.
+	if err := itr.Error(); err != nil {
+		itr.isInvalid = true
+		return false
+	}
 
 	// If source is invalid, invalid.
 	if !itr.source.Valid() {
@@ -94,7 +97,6 @@ func (itr *goLevelDBIterator) Valid() bool {
 func (itr *goLevelDBIterator) Key() []byte {
 	// Key returns a copy of the current key.
 	// See https://github.com/syndtr/goleveldb/blob/52c212e6c196a1404ea59592d3f1c227c9f034b2/leveldb/iterator/iter.go#L88
-	itr.assertNoError()
 	itr.assertIsValid()
 	return cp(itr.source.Key())
 }
@@ -103,14 +105,12 @@ func (itr *goLevelDBIterator) Key() []byte {
 func (itr *goLevelDBIterator) Value() []byte {
 	// Value returns a copy of the current value.
 	// See https://github.com/syndtr/goleveldb/blob/52c212e6c196a1404ea59592d3f1c227c9f034b2/leveldb/iterator/iter.go#L88
-	itr.assertNoError()
 	itr.assertIsValid()
 	return cp(itr.source.Value())
 }
 
 // Next implements Iterator.
 func (itr *goLevelDBIterator) Next() {
-	itr.assertNoError()
 	itr.assertIsValid()
 	if itr.isReverse {
 		itr.source.Prev()
@@ -125,19 +125,13 @@ func (itr *goLevelDBIterator) Error() error {
 }
 
 // Close implements Iterator.
-func (itr *goLevelDBIterator) Close() {
+func (itr *goLevelDBIterator) Close() error {
 	itr.source.Release()
-}
-
-func (itr *goLevelDBIterator) assertNoError() {
-	err := itr.source.Error()
-	if err != nil {
-		panic(err)
-	}
+	return nil
 }
 
 func (itr goLevelDBIterator) assertIsValid() {
 	if !itr.Valid() {
-		panic("goLevelDBIterator is invalid")
+		panic("iterator is invalid")
 	}
 }

memdb_iterator.go

@@ -93,11 +93,12 @@ func newMemDBIterator(db *MemDB, start []byte, end []byte, reverse bool) *memDBI
 }
 
 // Close implements Iterator.
-func (i *memDBIterator) Close() {
+func (i *memDBIterator) Close() error {
 	i.cancel()
 	for range i.ch { // drain channel
 	}
 	i.item = nil
+	return nil
 }
 
 // Domain implements Iterator.
@@ -112,12 +113,11 @@ func (i *memDBIterator) Valid() bool {
 
 // Next implements Iterator.
 func (i *memDBIterator) Next() {
+	i.assertIsValid()
 	item, ok := <-i.ch
 	switch {
 	case ok:
 		i.item = item
-	case i.item == nil:
-		panic("called Next() on invalid iterator")
 	default:
 		i.item = nil
 	}
@@ -130,16 +130,18 @@ func (i *memDBIterator) Error() error {
 
 // Key implements Iterator.
 func (i *memDBIterator) Key() []byte {
-	if i.item == nil {
-		panic("called Key() on invalid iterator")
-	}
+	i.assertIsValid()
 	return i.item.key
 }
 
 // Value implements Iterator.
 func (i *memDBIterator) Value() []byte {
-	if i.item == nil {
-		panic("called Value() on invalid iterator")
-	}
+	i.assertIsValid()
 	return i.item.value
 }
+
+func (i *memDBIterator) assertIsValid() {
+	if !i.Valid() {
+		panic("iterator is invalid")
+	}
+}

prefixdb_iterator.go

@@ -1,6 +1,9 @@
 package db
 
-import "bytes"
+import (
+	"bytes"
+	"fmt"
+)
 
 // IteratePrefix is a convenience function for iterating over a key domain
 // restricted by prefix.
@@ -36,6 +39,7 @@ type prefixDBIterator struct {
 	end    []byte
 	source Iterator
 	valid  bool
+	err    error
 }
 
 var _ Iterator = (*prefixDBIterator)(nil)
@@ -73,14 +77,23 @@ func (itr *prefixDBIterator) Domain() (start []byte, end []byte) {
 
 // Valid implements Iterator.
 func (itr *prefixDBIterator) Valid() bool {
-	return itr.valid && itr.source.Valid()
+	if !itr.valid || itr.err != nil || !itr.source.Valid() {
+		return false
+	}
+
+	key := itr.source.Key()
+	if len(key) < len(itr.prefix) || !bytes.Equal(key[:len(itr.prefix)], itr.prefix) {
+		itr.err = fmt.Errorf("received invalid key from backend: %x (expected prefix %x)",
+			key, itr.prefix)
+		return false
+	}
+
+	return true
 }
 
 // Next implements Iterator.
 func (itr *prefixDBIterator) Next() {
-	if !itr.valid {
-		panic("prefixIterator invalid; cannot call Next()")
-	}
+	itr.assertIsValid()
 	itr.source.Next()
 
 	if !itr.source.Valid() || !bytes.HasPrefix(itr.source.Key(), itr.prefix) {
@@ -89,39 +102,33 @@ func (itr *prefixDBIterator) Next() {
 }
 
 // Next implements Iterator.
-func (itr *prefixDBIterator) Key() (key []byte) {
-	if !itr.valid {
-		panic("prefixIterator invalid; cannot call Key()")
-	}
-	key = itr.source.Key()
-	return stripPrefix(key, itr.prefix)
+func (itr *prefixDBIterator) Key() []byte {
+	itr.assertIsValid()
+	key := itr.source.Key()
+	return key[len(itr.prefix):] // we have checked the key in Valid()
 }
 
 // Value implements Iterator.
-func (itr *prefixDBIterator) Value() (value []byte) {
-	if !itr.valid {
-		panic("prefixIterator invalid; cannot call Value()")
-	}
-	value = itr.source.Value()
-	return value
+func (itr *prefixDBIterator) Value() []byte {
+	itr.assertIsValid()
+	return itr.source.Value()
 }
 
 // Error implements Iterator.
 func (itr *prefixDBIterator) Error() error {
-	return itr.source.Error()
+	if err := itr.source.Error(); err != nil {
+		return err
+	}
+	return itr.err
 }
 
 // Close implements Iterator.
-func (itr *prefixDBIterator) Close() {
-	itr.source.Close()
+func (itr *prefixDBIterator) Close() error {
+	return itr.source.Close()
 }
 
-func stripPrefix(key []byte, prefix []byte) (stripped []byte) {
-	if len(key) < len(prefix) {
-		panic("should not happen")
-	}
-	if !bytes.Equal(key[:len(prefix)], prefix) {
-		panic("should not happen")
+func (itr *prefixDBIterator) assertIsValid() {
+	if !itr.Valid() {
+		panic("iterator is invalid")
 	}
-	return key[len(prefix):]
 }