diff --git a/CHANGELOG.md b/CHANGELOG.md index 03501ac97..39ea18b1f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/boltdb_iterator.go b/boltdb_iterator.go index 95adc871f..fc5ba368f 100644 --- a/boltdb_iterator.go +++ b/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") } } diff --git a/cleveldb_iterator.go b/cleveldb_iterator.go index f0a4f4e37..04375d565 100644 --- a/cleveldb_iterator.go +++ b/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") } } diff --git a/goleveldb_iterator.go b/goleveldb_iterator.go index 3a13c4d6f..9cf9a5e15 100644 --- a/goleveldb_iterator.go +++ b/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") } } diff --git a/memdb_iterator.go b/memdb_iterator.go index f76d249b7..2a61e3757 100644 --- a/memdb_iterator.go +++ b/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") + } +} diff --git a/prefixdb_iterator.go b/prefixdb_iterator.go index 9b50d4cd4..0f8ab7df7 100644 --- a/prefixdb_iterator.go +++ b/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):] } diff --git a/remotedb/iterator.go b/remotedb/iterator.go index 77b252512..325dc5386 100644 --- a/remotedb/iterator.go +++ b/remotedb/iterator.go @@ -1,30 +1,33 @@ package remotedb import ( - "fmt" - db "github.com/tendermint/tm-db" protodb "github.com/tendermint/tm-db/remotedb/proto" ) func makeIterator(dic protodb.DB_IteratorClient) db.Iterator { - return &iterator{dic: dic} + itr := &iterator{dic: dic} + itr.Next() // We need to call Next to prime the iterator + return itr } func makeReverseIterator(dric protodb.DB_ReverseIteratorClient) db.Iterator { - return &reverseIterator{dric: dric} + rItr := &reverseIterator{dric: dric} + rItr.Next() // We need to call Next to prime the iterator + return rItr } type reverseIterator struct { dric protodb.DB_ReverseIteratorClient cur *protodb.Iterator + err error } var _ db.Iterator = (*iterator)(nil) // Valid implements Iterator. func (rItr *reverseIterator) Valid() bool { - return rItr.cur != nil && rItr.cur.Valid + return rItr.cur != nil && rItr.cur.Valid && rItr.err == nil } // Domain implements Iterator. @@ -40,33 +43,37 @@ func (rItr *reverseIterator) Next() { var err error rItr.cur, err = rItr.dric.Recv() if err != nil { - panic(fmt.Sprintf("RemoteDB.ReverseIterator.Next error: %v", err)) + rItr.err = err } } // Key implements Iterator. func (rItr *reverseIterator) Key() []byte { - if rItr.cur == nil { - panic("key does not exist") - } + rItr.assertIsValid() return rItr.cur.Key } // Value implements Iterator. func (rItr *reverseIterator) Value() []byte { - if rItr.cur == nil { - panic("key does not exist") - } + rItr.assertIsValid() return rItr.cur.Value } // Error implements Iterator. func (rItr *reverseIterator) Error() error { - return nil + return rItr.err } // Close implements Iterator. -func (rItr *reverseIterator) Close() {} +func (rItr *reverseIterator) Close() error { + return nil +} + +func (rItr *reverseIterator) assertIsValid() { + if !rItr.Valid() { + panic("iterator is invalid") + } +} // iterator implements the db.Iterator by retrieving // streamed iterators from the remote backend as @@ -75,13 +82,14 @@ func (rItr *reverseIterator) Close() {} type iterator struct { dic protodb.DB_IteratorClient cur *protodb.Iterator + err error } var _ db.Iterator = (*iterator)(nil) // Valid implements Iterator. func (itr *iterator) Valid() bool { - return itr.cur != nil && itr.cur.Valid + return itr.cur != nil && itr.cur.Valid && itr.err == nil } // Domain implements Iterator. @@ -97,35 +105,34 @@ func (itr *iterator) Next() { var err error itr.cur, err = itr.dic.Recv() if err != nil { - panic(fmt.Sprintf("remoteDB.Iterator.Next error: %v", err)) + itr.err = err } } // Key implements Iterator. func (itr *iterator) Key() []byte { - if itr.cur == nil { - return nil - } + itr.assertIsValid() return itr.cur.Key } // Value implements Iterator. func (itr *iterator) Value() []byte { - if itr.cur == nil { - panic("current poisition is not valid") - } + itr.assertIsValid() return itr.cur.Value } // Error implements Iterator. func (itr *iterator) Error() error { - return nil + return itr.err } // Close implements Iterator. -func (itr *iterator) Close() { - err := itr.dic.CloseSend() - if err != nil { - panic(fmt.Sprintf("Error closing iterator: %v", err)) +func (itr *iterator) Close() error { + return itr.dic.CloseSend() +} + +func (itr *iterator) assertIsValid() { + if !itr.Valid() { + panic("iterator is invalid") } } diff --git a/remotedb/remotedb_test.go b/remotedb/remotedb_test.go index 01af129e4..8407b3004 100644 --- a/remotedb/remotedb_test.go +++ b/remotedb/remotedb_test.go @@ -5,6 +5,7 @@ import ( "os" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/tendermint/tm-db/remotedb" @@ -46,8 +47,7 @@ func TestRemoteDB(t *testing.T) { // Simple iteration itr, err := client.Iterator(nil, nil) require.NoError(t, err) - - itr.Next() + assert.True(t, itr.Valid()) key1 := itr.Key() value := itr.Value() @@ -72,10 +72,7 @@ func TestRemoteDB(t *testing.T) { itr, err = client.Iterator(nil, nil) require.NoError(t, err) - itr.Next() - key1 = itr.Key() - value = itr.Value() require.Equal(t, key1, []byte("key-1")) diff --git a/rocksdb_iterator.go b/rocksdb_iterator.go index 301e49d15..0e7d405fe 100644 --- a/rocksdb_iterator.go +++ b/rocksdb_iterator.go @@ -49,20 +49,23 @@ func newRocksDBIterator(source *gorocksdb.Iterator, start, end []byte, isReverse } // Domain implements Iterator. -func (itr rocksDBIterator) Domain() ([]byte, []byte) { +func (itr *rocksDBIterator) Domain() ([]byte, []byte) { return itr.start, itr.end } // Valid implements Iterator. -func (itr rocksDBIterator) Valid() bool { +func (itr *rocksDBIterator) Valid() bool { // Once invalid, forever invalid. if itr.isInvalid { return false } - // Panic on DB error. No way to recover. - itr.assertNoError() + // If source has error, invalid. + if err := itr.source.Err(); err != nil { + itr.isInvalid = true + return false + } // If source is invalid, invalid. if !itr.source.Valid() { @@ -91,22 +94,19 @@ func (itr rocksDBIterator) Valid() bool { } // Key implements Iterator. -func (itr rocksDBIterator) Key() []byte { - itr.assertNoError() +func (itr *rocksDBIterator) Key() []byte { itr.assertIsValid() return moveSliceToBytes(itr.source.Key()) } // Value implements Iterator. -func (itr rocksDBIterator) Value() []byte { - itr.assertNoError() +func (itr *rocksDBIterator) Value() []byte { itr.assertIsValid() return moveSliceToBytes(itr.source.Value()) } // Next implements Iterator. func (itr rocksDBIterator) Next() { - itr.assertNoError() itr.assertIsValid() if itr.isReverse { itr.source.Prev() @@ -116,24 +116,19 @@ func (itr rocksDBIterator) Next() { } // Error implements Iterator. -func (itr rocksDBIterator) Error() error { +func (itr *rocksDBIterator) Error() error { return itr.source.Err() } // Close implements Iterator. -func (itr rocksDBIterator) Close() { +func (itr *rocksDBIterator) Close() error { itr.source.Close() + return nil } -func (itr rocksDBIterator) assertNoError() { - if err := itr.source.Err(); err != nil { - panic(err) - } -} - -func (itr rocksDBIterator) assertIsValid() { +func (itr *rocksDBIterator) assertIsValid() { if !itr.Valid() { - panic("rocksDBIterator is invalid") + panic("iterator is invalid") } } diff --git a/types.go b/types.go index 6a373293b..e180efd7a 100644 --- a/types.go +++ b/types.go @@ -93,6 +93,9 @@ type Batch interface { // No writes can happen to a domain while there exists an iterator over it, some backends may take // out database locks to ensure this will not happen. // +// Callers must make sure the iterator is valid before calling any methods on it, otherwise +// these methods will panic. This is in part caused by most backend databases using this convention. +// // As with DB, keys and values should be considered read-only, and must be copied before they are // modified. // @@ -105,6 +108,9 @@ type Batch interface { // k, v := itr.Key(); itr.Value() // ... // } +// if err := itr.Error(); err != nil { +// ... +// } type Iterator interface { // Domain returns the start (inclusive) and end (exclusive) limits of the iterator. // CONTRACT: start, end readonly []byte @@ -130,5 +136,5 @@ type Iterator interface { Error() error // Close closes the iterator, relasing any allocated resources. - Close() + Close() error }