Bug 834449 - Part 3: simplify cleanup of cached statements. r=gps

toolkit/modules/Sqlite.jsm

@@ -204,10 +204,6 @@ function OpenedConnection(connection, basename, number, options) {
   // canceling prior to finalizing the mozIStorageStatements.
   this._pendingStatements = new Map();
 
-  // A map from statement index to mozIStorageStatement, for all outstanding
-  // query executions.
-  this._inProgressStatements = new Map();
-
   // Increments for each executed statement for the life of the connection.
   this._statementCounter = 0;
 
@@ -325,64 +321,6 @@ OpenedConnection.prototype = Object.freeze({
     return deferred.promise;
   },
 
-  /**
-   * Record that an executing statement has completed by removing it from the
-   * pending statements map and decrementing the corresponding in-progress
-   * counter.
-   */
-  _recordStatementCompleted: function (statement, index) {
-    this._log.debug("Stmt #" + index + " finished.");
-    this._pendingStatements.delete(index);
-
-    let now = this._inProgressStatements.get(statement) - 1;
-    if (now == 0) {
-      this._inProgressStatements.delete(statement);
-    } else {
-      this._inProgressStatements.set(statement, now);
-    }
-  },
-
-  /**
-   * Record that an executing statement is beginning by adding it to the
-   * pending statements map and incrementing the corresponding in-progress
-   * counter.
-   */
-  _recordStatementBeginning: function (statement, pending, index) {
-    this._log.info("Recording statement beginning: " + statement);
-    this._pendingStatements.set(index, pending);
-    if (!this._inProgressStatements.has(statement)) {
-      this._inProgressStatements.set(statement, 1);
-      this._log.info("Only one: " + statement);
-      return;
-    }
-    let now = this._inProgressStatements.get(statement) + 1;
-    this._inProgressStatements.set(statement, now);
-    this._log.info("More: " + statement + ", " + now);
-  },
-
-  /**
-   * Get a count of in-progress statements. This is useful for debugging and
-   * testing, and also for discarding cached statements.
-   *
-   * @param statement (optional) the statement after which to inquire.
-   * @return (integer) a count of executing queries, whether for `statement` or
-   *                   for the connection as a whole.
-   */
-  inProgress: function (statement) {
-    if (statement) {
-      if (this._inProgressStatements.has(statement)) {
-        return this._inProgressStatements.get(statement);
-      }
-      return 0;
-    }
-
-    let out = 0;
-    for (let v of this._inProgressStatements.values()) {
-      out += v;
-    }
-    return out;
-  },
-
   _finalize: function (deferred) {
     this._log.debug("Finalizing connection.");
     // Cancel any pending statements.
@@ -392,7 +330,6 @@ OpenedConnection.prototype = Object.freeze({
     this._pendingStatements.clear();
 
     // We no longer need to track these.
-    this._inProgressStatements.clear();
     this._statementCounter = 0;
 
     // Next we finalize all active statements.
@@ -732,21 +669,22 @@ OpenedConnection.prototype = Object.freeze({
   },
 
   /**
-   * Discard all inactive cached statements.
+   * Discard all cached statements.
+   *
+   * Note that this relies on us being non-interruptible between
+   * the insertion or retrieval of a statement in the cache and its
+   * execution: we finalize all statements, which is only safe if
+   * they will not be executed again.
    *
    * @return (integer) the number of statements discarded.
    */
   discardCachedStatements: function () {
     let count = 0;
     for (let [k, statement] of this._cachedStatements) {
-      if (this.inProgress(statement)) {
-        continue;
-      }
-
       ++count;
-      this._cachedStatements.delete(k);
       statement.finalize();
     }
+    this._cachedStatements.clear();
     this._log.debug("Discarded " + count + " cached statements.");
     return count;
   },
@@ -824,7 +762,8 @@ OpenedConnection.prototype = Object.freeze({
       },
 
       handleCompletion: function (reason) {
-        self._recordStatementCompleted(statement, index);
+        self._log.debug("Stmt #" + index + " finished.");
+        self._pendingStatements.delete(index);
 
         switch (reason) {
           case Ci.mozIStorageStatementCallback.REASON_FINISHED:
@@ -859,7 +798,7 @@ OpenedConnection.prototype = Object.freeze({
       },
     });
 
-    this._recordStatementBeginning(statement, pending, index);
+    this._pendingStatements.set(index, pending);
     return deferred.promise;
   },
 

toolkit/modules/tests/xpcshell/test_sqlite.js

@@ -478,10 +478,10 @@ add_task(function test_idle_shrink_reset_on_operation() {
 add_task(function test_in_progress_counts() {
   let c = yield getDummyDatabase("in_progress_counts");
   do_check_eq(c._statementCounter, c._initialStatementCount);
-  do_check_eq(c.inProgress(), 0);
+  do_check_eq(c._pendingStatements.size, 0);
   yield c.executeCached("INSERT INTO dirs (path) VALUES ('foo')");
   do_check_eq(c._statementCounter, c._initialStatementCount + 1);
-  do_check_eq(c.inProgress(), 0);
+  do_check_eq(c._pendingStatements.size, 0);
 
   let expectOne;
   let expectTwo;
@@ -491,7 +491,7 @@ add_task(function test_in_progress_counts() {
   let outer = Async.makeSpinningCallback();
 
   // We want to make sure that two queries executing simultaneously
-  // result in `inProgress()` reaching 2, then dropping back to 0.
+  // result in `_pendingStatements.size` reaching 2, then dropping back to 0.
   //
   // To do so, we kick off a second statement within the row handler
   // of the first, then wait for both to finish.
@@ -499,12 +499,12 @@ add_task(function test_in_progress_counts() {
   yield c.executeCached("SELECT * from dirs", null, function onRow() {
     // In the onRow handler, we're still an outstanding query.
     // Expect a single in-progress entry.
-    expectOne = c.inProgress();
+    expectOne = c._pendingStatements.size;
 
     // Start another query, checking that after its statement has been created
     // there are two statements in progress.
     let p = c.executeCached("SELECT 10, path from dirs");
-    expectTwo = c.inProgress();
+    expectTwo = c._pendingStatements.size;
 
     // Now wait for it to be done before we return from the row handler …
     p.then(function onInner() {
@@ -523,7 +523,7 @@ add_task(function test_in_progress_counts() {
   do_check_eq(expectOne, 1);
   do_check_eq(expectTwo, 2);
   do_check_eq(c._statementCounter, c._initialStatementCount + 3);
-  do_check_eq(c.inProgress(), 0);
+  do_check_eq(c._pendingStatements.size, 0);
 
   yield c.close();
 });
@@ -545,11 +545,8 @@ add_task(function test_discard_while_active() {
     discarded = c.discardCachedStatements();
   });
 
-  // We didn't discard the SELECT, only the two INSERTs.
-  do_check_eq(2, discarded);
-
-  // But we got the remainder when it became inactive.
-  do_check_eq(1, c.discardCachedStatements());
+  // We discarded everything, because the SELECT had already started to run.
+  do_check_eq(3, discarded);
 
   // And again is safe.
   do_check_eq(0, c.discardCachedStatements());