From 3af945692e1521b66a18f11b85ca0cfe8547db50 Mon Sep 17 00:00:00 2001
From: stasatdaglabs <39559713+stasatdaglabs@users.noreply.github.com>
Date: Thu, 23 Apr 2020 16:55:25 +0300
Subject: [PATCH] [NOD-922] Panic from cursor Next and First (#703)

* [NOD-922] Panic in Cursor First and Next if the cursor is closed.

* [NOD-922] Fix broken tests.

* [NOD-922] Fix a comment.
---
 database/cursor.go           |  4 +--
 database/cursor_test.go      | 52 +++++++++++++++++++++++-------------
 database/ffldb/ldb/cursor.go |  8 +++---
 3 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/database/cursor.go b/database/cursor.go
index 4f2d1a9e6..91a4bd807 100644
--- a/database/cursor.go
+++ b/database/cursor.go
@@ -3,11 +3,11 @@ package database
 // Cursor iterates over database entries given some bucket.
 type Cursor interface {
 	// Next moves the iterator to the next key/value pair. It returns whether the
-	// iterator is exhausted. Returns false if the cursor is closed.
+	// iterator is exhausted. Panics if the cursor is closed.
 	Next() bool
 
 	// First moves the iterator to the first key/value pair. It returns false if
-	// such a pair does not exist or if the cursor is closed.
+	// such a pair does not exist. Panics if the cursor is closed.
 	First() bool
 
 	// Seek moves the iterator to the first key/value pair whose key is greater
diff --git a/database/cursor_test.go b/database/cursor_test.go
index 492425984..c89c6fe4f 100644
--- a/database/cursor_test.go
+++ b/database/cursor_test.go
@@ -7,6 +7,7 @@ package database_test
 
 import (
 	"bytes"
+	"fmt"
 	"github.com/kaspanet/kaspad/database"
 	"reflect"
 	"strings"
@@ -23,6 +24,20 @@ func prepareCursorForTest(t *testing.T, db database.Database, testName string) d
 	return cursor
 }
 
+func recoverFromClosedCursorPanic(t *testing.T, testName string) {
+	panicErr := recover()
+	if panicErr == nil {
+		t.Fatalf("%s: cursor unexpectedly "+
+			"didn't panic after being closed", testName)
+	}
+	expectedPanicErr := "closed cursor"
+	if !strings.Contains(fmt.Sprintf("%v", panicErr), expectedPanicErr) {
+		t.Fatalf("%s: cursor panicked "+
+			"with wrong message. Want: %v, got: %s",
+			testName, expectedPanicErr, panicErr)
+	}
+}
+
 func TestCursorNext(t *testing.T) {
 	testForAllDatabaseTypes(t, "TestCursorNext", testCursorNext)
 }
@@ -67,19 +82,20 @@ func testCursorNext(t *testing.T, db database.Database, testName string) {
 			"not done", testName)
 	}
 
-	// Rewind the cursor, close it, and call Next on it again.
-	// This time it should return false because it's closed.
+	// Rewind the cursor and close it
 	cursor.First()
 	err := cursor.Close()
 	if err != nil {
 		t.Fatalf("%s: Close unexpectedly "+
 			"failed: %s", testName, err)
 	}
-	hasNext = cursor.Next()
-	if hasNext {
-		t.Fatalf("%s: cursor unexpectedly "+
-			"returned true after being closed", testName)
-	}
+
+	// Call Next on the cursor. This time it should panic
+	// because it's closed.
+	func() {
+		defer recoverFromClosedCursorPanic(t, testName)
+		cursor.Next()
+	}()
 }
 
 func TestCursorFirst(t *testing.T) {
@@ -315,17 +331,15 @@ func testCursorCloseFirstAndNext(t *testing.T, db database.Database, testName st
 			"unexpectedly failed: %s", testName, err)
 	}
 
-	// We expect First to return false
-	result := cursor.First()
-	if result {
-		t.Fatalf("%s: First "+
-			"unexpectedly returned true", testName)
-	}
+	// We expect First to panic
+	func() {
+		defer recoverFromClosedCursorPanic(t, testName)
+		cursor.First()
+	}()
 
-	// We expect Next to return false
-	result = cursor.Next()
-	if result {
-		t.Fatalf("%s: Next "+
-			"unexpectedly returned true", testName)
-	}
+	// We expect Next to panic
+	func() {
+		defer recoverFromClosedCursorPanic(t, testName)
+		cursor.Next()
+	}()
 }
diff --git a/database/ffldb/ldb/cursor.go b/database/ffldb/ldb/cursor.go
index e2b17ed79..a9d1eb3d2 100644
--- a/database/ffldb/ldb/cursor.go
+++ b/database/ffldb/ldb/cursor.go
@@ -27,19 +27,19 @@ func (db *LevelDB) Cursor(bucket *database.Bucket) *LevelDBCursor {
 }
 
 // Next moves the iterator to the next key/value pair. It returns whether the
-// iterator is exhausted. Returns false if the cursor is closed.
+// iterator is exhausted. Panics if the cursor is closed.
 func (c *LevelDBCursor) Next() bool {
 	if c.isClosed {
-		return false
+		panic("cannot call next on a closed cursor")
 	}
 	return c.ldbIterator.Next()
 }
 
 // First moves the iterator to the first key/value pair. It returns false if
-// such a pair does not exist or if the cursor is closed.
+// such a pair does not exist. Panics if the cursor is closed.
 func (c *LevelDBCursor) First() bool {
 	if c.isClosed {
-		return false
+		panic("cannot call first on a closed cursor")
 	}
 	return c.ldbIterator.First()
 }