From a631e25dc517e5473ea70863bae8633e0232ce00 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 19 Nov 2021 13:14:42 -0600
Subject: [PATCH] refactor: removeTx responsible for getting/releasing its own
 lock

We change nothing substantive here, except that there's a window
between when a write transaction updates the root pages and when
it removes itself from the db tx list and possibly causes a checkpoint
where it's not holding the db lock.

The issue here is that we want to be able to *keep* the lock but
still return, so no one else can start transactions, but the specific
Rollback or Commit that removed the last outstanding transaction
doesn't block forever. This will, later, allow us to exercise
finer-grained control over when we allow transactions. This is
a separate commit so we can run the test suite against it, and
verify that this part in particular didn't break anything.
---
 rbf/db.go |  7 ++++++-
 rbf/tx.go | 14 +++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index 65794c9eb..7f27ca418 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -503,8 +503,13 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 	return tx, nil
 }
 
-// removeTx removes an active transaction from the database.
+// removeTx removes an active transaction from the database. it obtains
+// the db lock, and currently drops it, but will later possibly be leaving
+// it retained by an asynchronous op that wants to happen before we start
+// running new tx.
 func (db *DB) removeTx(tx *Tx) error {
+	db.mu.Lock()
+	defer db.mu.Unlock()
 	// Release writer lock if tx is writable.
 	if tx.writable {
 		tx.db.rwmu.Unlock()
diff --git a/rbf/tx.go b/rbf/tx.go
index 38fed48f6..2cf7420db 100644
--- a/rbf/tx.go
+++ b/rbf/tx.go
@@ -109,20 +109,24 @@ func (tx *Tx) Commit() error {
 		// future plan: after checkpoint is moved to background
 		// or not every removeTx, then we can move the
 		// tx.db.rootRecords = tx.rootRecords into removeTx().
-
+		//
+		// ... or maybe not: let's do that part here, and then removeTx
+		// may or may not start a checkpoint, possibly asynchronously.
+		//
 		// avoid race detector firing on a write race here
-		// vs the read of rootRecords at db.Begin()
+		// vs the read of rootRecords at db.Begin(), then release
+		// the lock, because we need removeTx to grab the lock to
+		// work, but if it wants to checkpoint, it wants to be able to return
+		// to us here and still be holding the lock.
 		tx.db.mu.Lock()
-		defer tx.db.mu.Unlock()
 		tx.db.rootRecords = tx.rootRecords
 		tx.db.pageMap = tx.pageMap
 		tx.db.walPageN = tx.walPageN
+		tx.db.mu.Unlock()
 		return tx.db.removeTx(tx)
 	}
 
 	// Disconnect transaction from DB.
-	tx.db.mu.Lock()
-	defer tx.db.mu.Unlock()
 	return tx.db.removeTx(tx)
 }