From 9e3e680be80f70a446ceb2e106d458dc2a26b843 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Sat, 25 Apr 2015 02:11:09 -0400
Subject: [PATCH] Rework DB transaction handling

Rollback callbacks weren't being properly called, and some other things were in
the wrong place, particularly with nested transactions.
---
 chrome/content/zotero/xpcom/db.js | 142 ++++++++++++++++--------------
 test/tests/dbTest.js              |  83 +++++++++++++++++
 2 files changed, 158 insertions(+), 67 deletions(-)
 create mode 100644 test/tests/dbTest.js

diff --git a/chrome/content/zotero/xpcom/db.js b/chrome/content/zotero/xpcom/db.js
index 9f386e6e6a..27c65cb6d3 100644
--- a/chrome/content/zotero/xpcom/db.js
+++ b/chrome/content/zotero/xpcom/db.js
@@ -465,6 +465,15 @@ Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(func
 				}
 			}
 			
+			try {
+				var result = yield Zotero.Promise.coroutine(func)();
+			}
+			catch (e) {
+				Zotero.debug("Rolled back nested async DB transaction", 5);
+				this._asyncTransactionNestingLevel = 0;
+				throw e;
+			}
+			
 			Zotero.debug("Decreasing async DB transaction level to "
 				+ --this._asyncTransactionNestingLevel, 5);
 			return result;
@@ -489,78 +498,77 @@ Zotero.DBConnection.prototype.executeTransaction = Zotero.Promise.coroutine(func
 				}
 			}
 			var result = yield conn.executeTransaction(func);
-			try {
-				Zotero.debug("Committed async DB transaction", 5);
-				
-				// Clear transaction time
-				if (this._transactionDate) {
-					this._transactionDate = null;
-				}
-				
-				if (options) {
-					// Function to run once transaction has been committed but before any
-					// permanent callbacks
-					if (options.onCommit) {
-						this._callbacks.current.commit.push(options.onCommit);
-					}
-					this._callbacks.current.rollback = [];
-					
-					if (options.vacuumOnCommit) {
-						Zotero.debug('Vacuuming database');
-						yield Zotero.DB.queryAsync('VACUUM');
-					}
-				}
-				
-				// Run temporary commit callbacks
-				var f;
-				while (f = this._callbacks.current.commit.shift()) {
-					yield Zotero.Promise.resolve(f());
-				}
-				
-				// Run commit callbacks
-				for (var i=0; i<this._callbacks.commit.length; i++) {
-					if (this._callbacks.commit[i]) {
-						yield this._callbacks.commit[i]();
-					}
-				}
-				
-				setTimeout(resolve, 0);
-				
-				return result;
+			Zotero.debug("Committed async DB transaction", 5);
+			
+			// Clear transaction time
+			if (this._transactionDate) {
+				this._transactionDate = null;
 			}
-			catch (e) {
-				Zotero.debug("Rolled back async DB transaction", 5);
-				Zotero.debug(e, 1);
-				
-				try {
-					if (options) {
-						// Function to run once transaction has been committed but before any
-						// permanent callbacks
-						if (options.onRollback) {
-							this._callbacks.current.rollback.push(options.onRollback);
-						}
-					}
-					
-					// Run temporary commit callbacks
-					var f;
-					while (f = this._callbacks.current.rollback.shift()) {
-						yield Zotero.Promise.resolve(f());
-					}
-					
-					// Run rollback callbacks
-					for (var i=0; i<this._callbacks.rollback.length; i++) {
-						if (this._callbacks.rollback[i]) {
-							yield Zotero.Promise.resolve(this._callbacks.rollback[i]());
-						}
-					}
-				}
-				finally {
-					setTimeout(reject, 0);
+			
+			if (options) {
+				// Function to run once transaction has been committed but before any
+				// permanent callbacks
+				if (options.onCommit) {
+					this._callbacks.current.commit.push(options.onCommit);
 				}
+				this._callbacks.current.rollback = [];
 				
-				throw e;
+				if (options.vacuumOnCommit) {
+					Zotero.debug('Vacuuming database');
+					yield Zotero.DB.queryAsync('VACUUM');
+				}
+			}
+			
+			// Run temporary commit callbacks
+			var f;
+			while (f = this._callbacks.current.commit.shift()) {
+				yield Zotero.Promise.resolve(f());
+			}
+			
+			// Run commit callbacks
+			for (var i=0; i<this._callbacks.commit.length; i++) {
+				if (this._callbacks.commit[i]) {
+					yield this._callbacks.commit[i]();
+				}
+			}
+			
+			setTimeout(resolve, 0);
+			
+			return result;
+		}
+	}
+	catch (e) {
+		Zotero.debug("Rolled back async DB transaction", 5);
+		Zotero.debug(e, 1);
+		
+		if (options) {
+			// Function to run once transaction has been committed but before any
+			// permanent callbacks
+			if (options.onRollback) {
+				this._callbacks.current.rollback.push(options.onRollback);
 			}
 		}
+		
+		// Run temporary commit callbacks
+		var f;
+		while (f = this._callbacks.current.rollback.shift()) {
+			yield Zotero.Promise.resolve(f());
+		}
+		
+		// Run rollback callbacks
+		for (var i=0; i<this._callbacks.rollback.length; i++) {
+			if (this._callbacks.rollback[i]) {
+				yield Zotero.Promise.resolve(this._callbacks.rollback[i]());
+			}
+		}
+		
+		if (reject) {
+			setTimeout(function () {
+				reject(e);
+			}, 0);
+		}
+		
+		throw e;
 	}
 	finally {
 		// Reset options back to their previous values
diff --git a/test/tests/dbTest.js b/test/tests/dbTest.js
new file mode 100644
index 0000000000..72b1457666
--- /dev/null
+++ b/test/tests/dbTest.js
@@ -0,0 +1,83 @@
+describe("Zotero.DB", function() {
+	describe("#executeTransaction()", function () {
+		it("should roll back on error", function* () {
+			yield Zotero.DB.queryAsync("CREATE TABLE tmpRollbackOnError (foo INT)");
+			yield Zotero.DB.queryAsync("INSERT INTO tmpRollbackOnError VALUES (1)");
+			try {
+				yield Zotero.DB.executeTransaction(function* () {
+					yield Zotero.DB.queryAsync("INSERT INTO tmpRollbackOnError VALUES (2)");
+					throw 'Aborting transaction -- ignore';
+				});
+			}
+			catch (e) {
+				if (typeof e != 'string' || !e.startsWith('Aborting transaction')) throw e;
+			}
+			var count = yield Zotero.DB.valueQueryAsync("SELECT COUNT(*) FROM tmpRollbackOnError");
+			assert.equal(count, 1);
+			
+			var conn = yield Zotero.DB._getConnectionAsync();
+			assert.isFalse(conn.transactionInProgress);
+			
+			yield Zotero.DB.queryAsync("DROP TABLE tmpRollbackOnError");
+		});
+		
+		it("should run onRollback callbacks", function* () {
+			var callbackRan = false;
+			yield Zotero.DB.queryAsync("CREATE TABLE tmpOnRollback (foo INT)");
+			try {
+				yield Zotero.DB.executeTransaction(
+					function* () {
+						yield Zotero.DB.queryAsync("INSERT INTO tmpOnRollback VALUES (1)");
+						throw 'Aborting transaction -- ignore';
+					},
+					{
+						onRollback: function () {
+							callbackRan = true;
+						}
+					}
+				);
+			}
+			catch (e) {
+				if (typeof e != 'string' || !e.startsWith('Aborting transaction')) throw e;
+			}
+			assert.ok(callbackRan);
+			
+			yield Zotero.DB.queryAsync("DROP TABLE tmpOnRollback");
+		});
+		
+		it("should run onRollback callbacks for nested transactions", function* () {
+			var callback1Ran = false;
+			var callback2Ran = false;
+			yield Zotero.DB.queryAsync("CREATE TABLE tmpOnNestedRollback (foo INT)");
+			try {
+				yield Zotero.DB.executeTransaction(function* () {
+					yield Zotero.DB.queryAsync("INSERT INTO tmpOnNestedRollback VALUES (1)");
+					yield Zotero.DB.executeTransaction(
+						function* () {
+							yield Zotero.DB.queryAsync("INSERT INTO tmpOnNestedRollback VALUES (2)");
+							
+							throw 'Aborting transaction -- ignore';
+						},
+						{
+							onRollback: function () {
+								callback1Ran = true;
+							}
+						}
+					);
+				},
+				{
+					onRollback: function () {
+						callback2Ran = true;
+					}
+				});
+			}
+			catch (e) {
+				if (typeof e != 'string' || !e.startsWith('Aborting transaction')) throw e;
+			}
+			assert.ok(callback1Ran);
+			assert.ok(callback2Ran);
+			
+			yield Zotero.DB.queryAsync("DROP TABLE tmpOnNestedRollback");
+		});
+	})
+});