Bug 830209 - SQLite.jsm now handles transactions off the main thread. r=mak, a=bajaj
authorGregory Szorc <gps@mozilla.com>
Fri, 18 Jan 2013 10:11:22 -0800
changeset 127288 128b57a6b77f54ebc0d0fd7cd0be73bc47024bad
parent 127287 07e1d48fd8f79edb8b0c8a08b0feb3090458a7b2
child 127289 f30fa0f310b2eac7f93e095a2ea5a0c55f525e98
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, bajaj
bugs830209
milestone20.0a2
Bug 830209 - SQLite.jsm now handles transactions off the main thread. r=mak, a=bajaj
toolkit/modules/Sqlite.jsm
toolkit/modules/tests/xpcshell/test_sqlite.js
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -171,19 +171,21 @@ function OpenedConnection(connection, ba
   this._anonymousCounter = 0;
   this._inProgressStatements = new Map();
   this._inProgressCounter = 0;
 
   this._inProgressTransaction = null;
 }
 
 OpenedConnection.prototype = Object.freeze({
-  TRANSACTION_DEFERRED: Ci.mozIStorageConnection.TRANSACTION_DEFERRED,
-  TRANSACTION_IMMEDIATE: Ci.mozIStorageConnection.TRANSACTION_IMMEDIATE,
-  TRANSACTION_EXCLUSIVE: Ci.mozIStorageConnection.TRANSACTION_EXCLUSIVE,
+  TRANSACTION_DEFERRED: "DEFERRED",
+  TRANSACTION_IMMEDIATE: "IMMEDIATE",
+  TRANSACTION_EXCLUSIVE: "EXCLUSIVE",
+
+  TRANSACTION_TYPES: ["DEFERRED", "IMMEDIATE", "EXCLUSIVE"],
 
   get connectionReady() {
     return this._open && this._connection.connectionReady;
   },
 
   /**
    * The row ID from the last INSERT operation.
    *
@@ -244,31 +246,45 @@ OpenedConnection.prototype = Object.free
    *
    * @return Promise<>
    */
   close: function () {
     if (!this._connection) {
       return Promise.resolve();
     }
 
-    this._log.debug("Closing.");
+    this._log.debug("Request to close connection.");
+
+    let deferred = Promise.defer();
 
-    // Abort in-progress transaction.
-    if (this._inProgressTransaction) {
-      this._log.warn("Transaction in progress at time of close.");
-      try {
-        this._connection.rollbackTransaction();
-      } catch (ex) {
-        this._log.warn("Error rolling back transaction: " +
-                       CommonUtils.exceptionStr(ex));
-      }
-      this._inProgressTransaction.reject(new Error("Connection being closed."));
-      this._inProgressTransaction = null;
+    // We need to take extra care with transactions during shutdown.
+    //
+    // If we don't have a transaction in progress, we can proceed with shutdown
+    // immediately.
+    if (!this._inProgressTransaction) {
+      this._finalize(deferred);
+      return deferred.promise;
     }
 
+    // Else if we do have a transaction in progress, we forcefully roll it
+    // back. This is an async task, so we wait on it to finish before
+    // performing finalization.
+    this._log.warn("Transaction in progress at time of close. Rolling back.");
+
+    let onRollback = this._finalize.bind(this, deferred);
+
+    this.execute("ROLLBACK TRANSACTION").then(onRollback, onRollback);
+    this._inProgressTransaction.reject(new Error("Connection being closed."));
+    this._inProgressTransaction = null;
+
+    return deferred.promise;
+  },
+
+  _finalize: function (deferred) {
+    this._log.debug("Finalizing connection.");
     // Cancel any in-progress statements.
     for (let [k, statement] of this._inProgressStatements) {
       statement.cancel();
     }
     this._inProgressStatements.clear();
 
     // Next we finalize all active statements.
     for (let [k, statement] of this._anonymousStatements) {
@@ -280,28 +296,24 @@ OpenedConnection.prototype = Object.free
       statement.finalize();
     }
     this._cachedStatements.clear();
 
     // This guards against operations performed between the call to this
     // function and asyncClose() finishing. See also bug 726990.
     this._open = false;
 
-    let deferred = Promise.defer();
-
     this._log.debug("Calling asyncClose().");
     this._connection.asyncClose({
       complete: function () {
         this._log.info("Closed");
         this._connection = null;
         deferred.resolve();
       }.bind(this),
     });
-
-    return deferred.promise;
   },
 
   /**
    * Execute a SQL statement and cache the underlying statement object.
    *
    * This function executes a SQL statement and also caches the underlying
    * derived statement object so subsequent executions are faster and use
    * less resources.
@@ -418,17 +430,17 @@ OpenedConnection.prototype = Object.free
 
     return deferred.promise;
   },
 
   /**
    * Whether a transaction is currently in progress.
    */
   get transactionInProgress() {
-    return this._open && this._connection.transactionInProgress;
+    return this._open && !!this._inProgressTransaction;
   },
 
   /**
    * Perform a transaction.
    *
    * A transaction is specified by a user-supplied function that is a
    * generator function which can be used by Task.jsm's Task.spawn(). The
    * function receives this connection instance as its argument.
@@ -445,44 +457,86 @@ OpenedConnection.prototype = Object.free
    * the transaction is rolled back, the promise is rejected.
    *
    * @param func
    *        (function) What to perform as part of the transaction.
    * @param type optional
    *        One of the TRANSACTION_* constants attached to this type.
    */
   executeTransaction: function (func, type=this.TRANSACTION_DEFERRED) {
+    if (this.TRANSACTION_TYPES.indexOf(type) == -1) {
+      throw new Error("Unknown transaction type: " + type);
+    }
+
     this._ensureOpen();
 
-    if (this.transactionInProgress) {
+    if (this._inProgressTransaction) {
       throw new Error("A transaction is already active. Only one transaction " +
                       "can be active at a time.");
     }
 
     this._log.debug("Beginning transaction");
-    this._connection.beginTransactionAs(type);
-
     let deferred = Promise.defer();
     this._inProgressTransaction = deferred;
+    Task.spawn(function doTransaction() {
+      // It's tempting to not yield here and rely on the implicit serial
+      // execution of issued statements. However, the yield serves an important
+      // purpose: catching errors in statement execution.
+      yield this.execute("BEGIN " + type + " TRANSACTION");
 
-    Task.spawn(func(this)).then(
-      function onSuccess (result) {
-        this._connection.commitTransaction();
+      let result;
+      try {
+        result = yield Task.spawn(func(this));
+      } catch (ex) {
+        // It's possible that a request to close the connection caused the
+        // error.
+        // Assertion: close() will unset this._inProgressTransaction when
+        // called.
+        if (!this._inProgressTransaction) {
+          this._log.warn("Connection was closed while performing transaction. " +
+                         "Received error should be due to closed connection: " +
+                         CommonUtils.exceptionStr(ex));
+          throw ex;
+        }
+
+        this._log.warn("Error during transaction. Rolling back: " +
+                       CommonUtils.exceptionStr(ex));
+        try {
+          yield this.execute("ROLLBACK TRANSACTION");
+        } catch (inner) {
+          this._log.warn("Could not roll back transaction. This is weird: " +
+                         CommonUtils.exceptionStr(inner));
+        }
+
+        throw ex;
+      }
+
+      // See comment above about connection being closed during transaction.
+      if (!this._inProgressTransaction) {
+        this._log.warn("Connection was closed while performing transaction. " +
+                       "Unable to commit.");
+        throw new Error("Connection closed before transaction committed.");
+      }
+
+      try {
+        yield this.execute("COMMIT TRANSACTION");
+      } catch (ex) {
+        this._log.warn("Error committing transaction: " +
+                       CommonUtils.exceptionStr(ex));
+        throw ex;
+      }
+
+      throw new Task.Result(result);
+    }.bind(this)).then(
+      function onSuccess(result) {
         this._inProgressTransaction = null;
-        this._log.debug("Transaction committed.");
-
         deferred.resolve(result);
       }.bind(this),
-
-      function onError (error) {
-        this._log.warn("Error during transaction. Rolling back: " +
-                       CommonUtils.exceptionStr(error));
-        this._connection.rollbackTransaction();
+      function onError(error) {
         this._inProgressTransaction = null;
-
         deferred.reject(error);
       }.bind(this)
     );
 
     return deferred.promise;
   },
 
   /**
--- a/toolkit/modules/tests/xpcshell/test_sqlite.js
+++ b/toolkit/modules/tests/xpcshell/test_sqlite.js
@@ -208,16 +208,32 @@ add_task(function test_on_row_stop_itera
   });
 
   do_check_null(result);
   do_check_eq(i, 5);
 
   yield c.close();
 });
 
+add_task(function test_invalid_transaction_type() {
+  let c = yield getDummyDatabase("invalid_transaction_type");
+
+  let errored = false;
+  try {
+    c.executeTransaction(function () {}, "foobar");
+  } catch (ex) {
+    errored = true;
+    do_check_true(ex.message.startsWith("Unknown transaction type"));
+  } finally {
+    do_check_true(errored);
+  }
+
+  yield c.close();
+});
+
 add_task(function test_execute_transaction_success() {
   let c = yield getDummyDatabase("execute_transaction_success");
 
   do_check_false(c.transactionInProgress);
 
   yield c.executeTransaction(function transaction(conn) {
     do_check_eq(c, conn);
     do_check_true(conn.transactionInProgress);
@@ -252,8 +268,58 @@ add_task(function test_execute_transacti
   yield deferred.promise;
 
   let rows = yield c.execute("SELECT * FROM dirs");
   do_check_eq(rows.length, 0);
 
   yield c.close();
 });
 
+add_task(function test_close_during_transaction() {
+  let c = yield getDummyDatabase("close_during_transaction");
+
+  yield c.execute("INSERT INTO dirs (path) VALUES ('foo')");
+
+  let errored = false;
+  try {
+    yield c.executeTransaction(function transaction(conn) {
+      yield c.execute("INSERT INTO dirs (path) VALUES ('bar')");
+      yield c.close();
+    });
+  } catch (ex) {
+    errored = true;
+    do_check_eq(ex.message, "Connection being closed.");
+  } finally {
+    do_check_true(errored);
+  }
+
+  let c2 = yield getConnection("close_during_transaction");
+  let rows = yield c2.execute("SELECT * FROM dirs");
+  do_check_eq(rows.length, 1);
+
+  yield c2.close();
+});
+
+add_task(function test_detect_multiple_transactions() {
+  let c = yield getDummyDatabase("detect_multiple_transactions");
+
+  yield c.executeTransaction(function main() {
+    yield c.execute("INSERT INTO dirs (path) VALUES ('foo')");
+
+    let errored = false;
+    try {
+      yield c.executeTransaction(function child() {
+        yield c.execute("INSERT INTO dirs (path) VALUES ('bar')");
+      });
+    } catch (ex) {
+      errored = true;
+      do_check_true(ex.message.startsWith("A transaction is already active."));
+    } finally {
+      do_check_true(errored);
+    }
+  });
+
+  let rows = yield c.execute("SELECT * FROM dirs");
+  do_check_eq(rows.length, 1);
+
+  yield c.close();
+});
+