Merge pull request ava-labs#409 from ava-labs/iterator-error-releasing

Added proper error reporting after iterator releasing in the rpcdb

database/rpcdb/db_client.go

@@ -261,8 +261,12 @@ func (it *iterator) Value() []byte { return it.value }
 
 // Release frees any resources held by the iterator
 func (it *iterator) Release() {
-	_, err := it.db.client.IteratorRelease(context.Background(), &rpcdbproto.IteratorReleaseRequest{
+	resp, err := it.db.client.IteratorRelease(context.Background(), &rpcdbproto.IteratorReleaseRequest{
 		Id: it.id,
 	})
-	it.errs.Add(err)
+	if err != nil {
+		it.errs.Add(err)
+	} else {
+		it.errs.Add(errCodeToError[resp.Err])
+	}
 }

database/rpcdb/db_server.go

@@ -162,9 +162,12 @@ func (db *DatabaseServer) IteratorRelease(_ context.Context, req *rpcdbproto.Ite
 	defer db.lock.Unlock()
 
 	it, exists := db.iterators[req.Id]
-	if exists {
-		delete(db.iterators, req.Id)
-		it.Release()
+	if !exists {
+		return &rpcdbproto.IteratorReleaseResponse{Err: 0}, nil
 	}
-	return &rpcdbproto.IteratorReleaseResponse{}, nil
+
+	delete(db.iterators, req.Id)
+	err := it.Error()
+	it.Release()
+	return &rpcdbproto.IteratorReleaseResponse{Err: errorToErrCode[err]}, errorToRPCError(err)
 }

database/rpcdb/rpcdbproto/rpcdb.proto

@@ -102,7 +102,9 @@ message IteratorReleaseRequest {
     uint64 id = 1;
 }
 
-message IteratorReleaseResponse {}
+message IteratorReleaseResponse {
+    uint32 err = 1;
+}
 
 service Database {
     rpc Has(HasRequest) returns (HasResponse);

database/test_database.go

@@ -26,6 +26,8 @@ var (
 		TestIteratorStartPrefix,
 		TestIteratorMemorySafety,
 		TestIteratorClosed,
+		TestIteratorError,
+		TestIteratorErrorAfterRelease,
 		TestStatNoPanic,
 		TestCompactNoPanic,
 		TestMemorySafetyDatabase,
@@ -805,6 +807,75 @@ func TestIteratorClosed(t *testing.T, db Database) {
 	}
 }
 
+// TestIteratorError tests to make sure that an iterator still works after the
+// database is closed.
+func TestIteratorError(t *testing.T, db Database) {
+	key := []byte("hello1")
+	value := []byte("world1")
+
+	if err := db.Put(key, value); err != nil {
+		t.Fatalf("Unexpected error on batch.Put: %s", err)
+	}
+
+	iterator := db.NewIterator()
+	if iterator == nil {
+		t.Fatalf("db.NewIterator returned nil")
+	}
+	defer iterator.Release()
+
+	if err := db.Close(); err != nil {
+		t.Fatalf("Unexpected error on db.Close: %s", err)
+	}
+
+	if !iterator.Next() {
+		t.Fatalf("iterator.Next Returned: %v ; Expected: %v", false, true)
+	}
+	if itKey := iterator.Key(); !bytes.Equal(itKey, key) {
+		t.Fatalf("iterator.Key Returned: 0x%x ; Expected: 0x%x", itKey, key)
+	}
+	if itValue := iterator.Value(); !bytes.Equal(itValue, value) {
+		t.Fatalf("iterator.Value Returned: 0x%x ; Expected: 0x%x", itValue, value)
+	}
+	if err := iterator.Error(); err != nil {
+		t.Fatalf("Expected no error on iterator.Error but got %s", err)
+	}
+}
+
+// TestIteratorErrorAfterRelease tests to make sure that an iterator that was
+// released still reports the error correctly.
+func TestIteratorErrorAfterRelease(t *testing.T, db Database) {
+	key := []byte("hello1")
+	value := []byte("world1")
+
+	if err := db.Put(key, value); err != nil {
+		t.Fatalf("Unexpected error on batch.Put: %s", err)
+	}
+
+	if err := db.Close(); err != nil {
+		t.Fatalf("Unexpected error on db.Close: %s", err)
+	}
+
+	iterator := db.NewIterator()
+	if iterator == nil {
+		t.Fatalf("db.NewIterator returned nil")
+	}
+
+	iterator.Release()
+
+	if iterator.Next() {
+		t.Fatalf("iterator.Next Returned: %v ; Expected: %v", false, true)
+	}
+	if key := iterator.Key(); key != nil {
+		t.Fatalf("iterator.Key Returned: 0x%x ; Expected: nil", key)
+	}
+	if value := iterator.Value(); value != nil {
+		t.Fatalf("iterator.Value Returned: 0x%x ; Expected: nil", value)
+	}
+	if err := iterator.Error(); err != ErrClosed {
+		t.Fatalf("Expected %s on iterator.Error", ErrClosed)
+	}
+}
+
 // TestStatNoPanic tests to make sure that Stat never panics.
 func TestStatNoPanic(t *testing.T, db Database) {
 	key1 := []byte("hello1")