sqlite: make sure Step always resets on error

This was causing the sqliteutil.Save release function to panic, so add a test for it there. Fixes #40.
crawshaw · Sep 18, 2018 · b99de4f · b99de4f
1 parent 0171be5
commit b99de4f
Show file tree

Hide file tree

Showing 2 changed files with 72 additions and 26 deletions.
diff --git a/sqlite.go b/sqlite.go
@@ -61,7 +61,7 @@ import (
 // A Conn can only be used by goroutine at a time.
 type Conn struct {
 	conn   *C.sqlite3
-	stmts  map[string]*Stmt
+	stmts  map[string]*Stmt // query -> prepared statement
 	closed bool
 	count  int // shared variable to help the race detector find Conn misuse
 
@@ -380,7 +380,6 @@ func (conn *Conn) extreserr(loc, query string, res C.int) error {
 	switch res {
 	case C.SQLITE_OK, C.SQLITE_ROW, C.SQLITE_DONE:
 		return nil
-	case C.SQLITE_BUSY:
 	default:
 		msg = C.GoString(C.sqlite3_errmsg(conn.conn))
 	}
@@ -475,11 +474,10 @@ func (stmt *Stmt) Reset() error {
 		// in the wild, but so far has eluded exact test case replication.
 		// TODO: write a test for this.
 		if res := C.wait_for_unlock_notify(stmt.conn.conn, stmt.conn.unlockNote); res != C.SQLITE_OK {
-			return stmt.lockedError("Stmt.Reset(Wait)", res)
+			return stmt.conn.extreserr("Stmt.Reset(Wait)", stmt.query, res)
 		}
-
 	}
-	return stmt.conn.reserr("Stmt.Reset", stmt.query, res)
+	return stmt.conn.extreserr("Stmt.Reset", stmt.query, res)
 }
 
 // ClearBindings clears all bound parameter values on a statement.
@@ -498,10 +496,16 @@ func (stmt *Stmt) ClearBindings() error {
 //
 // If a row of data is available, rowReturned is reported as true.
 // If the statement has reached the end of the available data then
-// rowReturned is false.
+// rowReturned is false. Thus the status codes SQLITE_ROW and
+// SQLITE_DONE are reported by the rowReturned bool, and all other
+// non-OK status codes are reported as an error.
+//
+// If an error value is returned, then the statement has been reset.
 //
 // https://www.sqlite.org/c3ref/step.html
 //
+// Shared cache
+//
 // As the sqlite package enables shared cache mode by default
 // and multiple writers are common in multi-threaded programs,
 // this Step method uses sqlite3_unlock_notify to handle any
@@ -529,35 +533,35 @@ func (stmt *Stmt) Step() (rowReturned bool, err error) {
 		return false, err
 	}
 	defer func() {
+		if err != nil {
+			C.sqlite3_reset(stmt.stmt)
+		}
 		stmt.lastHasRow = rowReturned
 	}()
 
 	for {
 		stmt.conn.count++
 		if err := stmt.interrupted("Stmt.Step"); err != nil {
-			stmt.Reset()
 			return false, err
 		}
 		switch res := C.sqlite3_step(stmt.stmt); uint8(res) { // reduce to non-extended error code
 		case C.SQLITE_LOCKED:
 			if res != C.SQLITE_LOCKED_SHAREDCACHE {
 				// don't call wait_for_unlock_notify as it might deadlock, see:
 				// https://github.com/crawshaw/sqlite/issues/6
-				stmt.Reset()
-
-				return false, stmt.lockedError("Stmt.Step", res)
+				return false, stmt.conn.extreserr("Stmt.Step", stmt.query, res)
 			}
 
 			if res := C.wait_for_unlock_notify(stmt.conn.conn, stmt.conn.unlockNote); res != C.SQLITE_OK {
-				return false, stmt.lockedError("Stmt.Step(Wait)", res)
+				return false, stmt.conn.extreserr("Stmt.Step(Wait)", stmt.query, res)
 			}
 			C.sqlite3_reset(stmt.stmt)
 			// loop
 		case C.SQLITE_ROW:
 			return true, nil
 		case C.SQLITE_DONE:
 			return false, nil
-		case C.SQLITE_BUSY, C.SQLITE_INTERRUPT, C.SQLITE_CONSTRAINT:
+		case C.SQLITE_INTERRUPT, C.SQLITE_CONSTRAINT:
 			// TODO: embed some of these errors into the stmt for zero-alloc errors?
 			return false, stmt.conn.reserr("Stmt.Step", stmt.query, res)
 		default:
@@ -566,20 +570,6 @@ func (stmt *Stmt) Step() (rowReturned bool, err error) {
 	}
 }
 
-func (stmt *Stmt) lockedError(loc string, res C.int) error {
-	msg := ""
-	for _, stmt := range stmt.conn.stmts {
-		if !stmt.lastHasRow {
-			continue
-		}
-		if msg == "" {
-			msg = "other open queries:"
-		}
-		msg += "\n\t" + stmt.query
-	}
-	return reserr(loc, stmt.query, msg, res)
-}
-
 func (stmt *Stmt) handleBindErr(loc string, res C.int) {
 	if stmt.bindErr == nil {
 		stmt.bindErr = stmt.conn.reserr(loc, stmt.query, res)

diff --git a/sqliteutil/savepoint_test.go b/sqliteutil/savepoint_test.go
@@ -17,6 +17,8 @@ package sqliteutil
 import (
 	"context"
 	"errors"
+	"io/ioutil"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -269,3 +271,57 @@ func TestInterruptRollback(t *testing.T) {
 		t.Errorf("want 1 row, got %d", got)
 	}
 }
+
+func TestBusySnapshot(t *testing.T) {
+	dir, err := ioutil.TempDir("", "sqliteutil-test-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	db := filepath.Join(dir, "busysnapshot.db")
+
+	// No SQLITE_OPEN_SHAREDCACHE.
+	flags := sqlite.SQLITE_OPEN_READWRITE | sqlite.SQLITE_OPEN_CREATE | sqlite.SQLITE_OPEN_WAL
+	conn0, err := sqlite.OpenConn(db, flags)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer conn0.Close()
+
+	conn1, err := sqlite.OpenConn(db, flags)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer conn1.Close()
+
+	if err = ExecScript(conn0, `
+		DROP TABLE IF EXISTS t;
+		CREATE TABLE t (c, b BLOB);
+		INSERT INTO t (c, b) VALUES (4, "hi");
+	`); err != nil {
+		t.Fatal(err)
+	}
+
+	conn0Release0 := Save(conn0)
+
+	c, err := ResultInt(conn0.Prep("SELECT count(*) FROM t WHERE c > 3;"))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// An insert on conn1 invalidates the deferred transaction on conn0.
+	if err := Exec(conn1, "INSERT INTO t (c) VALUES (4);", nil); err != nil {
+		t.Fatal(err)
+	}
+
+	stmt := conn0.Prep("UPDATE t SET c = $c WHERE c = 4;")
+	stmt.SetInt64("$c", int64(c))
+	_, conn0Err := stmt.Step()
+	if sqlite.ErrCode(conn0Err) != sqlite.SQLITE_BUSY_SNAPSHOT {
+		t.Fatalf("want SQLITE_BUSY_SNAPSHOT, got: %v", conn0Err)
+	}
+
+	conn0Release0(&conn0Err)
+	if sqlite.ErrCode(conn0Err) != sqlite.SQLITE_BUSY_SNAPSHOT {
+		t.Fatalf("after savepoint want SQLITE_BUSY_SNAPSHOT, got: %v", conn0Err)
+	}
+}