Bug 1442353 - Reuse timeoutPromise in Sqlite.jsm r?florian draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 01 Mar 2018 10:43:07 -0800
changeset 762673 8df68fb019a9a80676138f514d9465b54e8f8ee4
parent 762045 40ba247350dad945cccd29680ee53d578c006420
push id101229
push userbmo:tchiovoloni@mozilla.com
push dateFri, 02 Mar 2018 19:53:51 +0000
reviewersflorian
bugs1442353
milestone60.0a1
Bug 1442353 - Reuse timeoutPromise in Sqlite.jsm r?florian MozReview-Commit-ID: 6AlvYliZcmy
toolkit/modules/Sqlite.jsm
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -3,18 +3,20 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 var EXPORTED_SYMBOLS = [
   "Sqlite",
 ];
 
-// The time to wait before considering a transaction stuck and rejecting it.
-const TRANSACTIONS_QUEUE_TIMEOUT_MS = 240000; // 4 minutes
+// The maximum time to wait before considering a transaction stuck and rejecting
+// it. (Note that the minimum amount of time we wait is 20% less than this, see
+// the `_getTimeoutPromise` method on `ConnectionData` for details).
+const TRANSACTIONS_QUEUE_TIMEOUT_MS = 300000; // 5 minutes
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Timer.jsm");
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
   Services: "resource://gre/modules/Services.jsm",
   OS: "resource://gre/modules/osfile.jsm",
@@ -254,16 +256,24 @@ function ConnectionData(connection, iden
       identifier: this._identifier,
       isCloseRequested: this._closeRequested,
       hasDbConn: !!this._dbConn,
       hasInProgressTransaction: this._hasInProgressTransaction,
       pendingStatements: this._pendingStatements.size,
       statementCounter: this._statementCounter,
     })
   );
+
+  // We avoid creating a timer every transaction that exists solely as a safety
+  // check (e.g. one that never should fire) by reusing it if it's sufficiently
+  // close to when the previous promise was created (see bug 1442353 and
+  // `_getTimeoutPromise` for more info).
+  this._timeoutPromise = null;
+  // The last timestamp when we should consider using `this._timeoutPromise`.
+  this._timeoutPromiseExpires = 0;
 }
 
 /**
  * Map of connection identifiers to ConnectionData objects
  *
  * The connection identifier is a human-readable name of the
  * database. Used by finalization witnesses to be able to close opened
  * connections on garbage collection.
@@ -629,20 +639,17 @@ ConnectionData.prototype = Object.freeze
           this._hasInProgressTransaction = false;
         }
       })();
 
       // If a transaction yields on a never resolved promise, or is mistakenly
       // nested, it could hang the transactions queue forever.  Thus we timeout
       // the execution after a meaningful amount of time, to ensure in any case
       // we'll proceed after a while.
-      let timeoutPromise = new Promise((resolve, reject) => {
-        setTimeout(() => reject(new Error("Transaction timeout, most likely caused by unresolved pending work.")),
-                   TRANSACTIONS_QUEUE_TIMEOUT_MS);
-      });
+      let timeoutPromise = this._getTimeoutPromise();
       return Promise.race([transactionPromise, timeoutPromise]);
     });
     // Atomically update the queue before anyone else has a chance to enqueue
     // further transactions.
     this._transactionQueue = promise.catch(ex => { console.error(ex); });
 
     // Make sure that we do not shutdown the connection during a transaction.
     this._barrier.client.addBlocker(`Transaction (${this._getOperationId()})`,
@@ -838,16 +845,38 @@ ConnectionData.prototype = Object.freeze
   _startIdleShrinkTimer() {
     if (!this._idleShrinkTimer) {
       return;
     }
 
     this._idleShrinkTimer.initWithCallback(this.shrinkMemory.bind(this),
                                            this._idleShrinkMS,
                                            this._idleShrinkTimer.TYPE_ONE_SHOT);
+  },
+
+  // Returns a promise that will resolve after a time comprised between 80% of
+  // `TRANSACTIONS_QUEUE_TIMEOUT_MS` and `TRANSACTIONS_QUEUE_TIMEOUT_MS`. Use
+  // this promise instead of creating several individual timers to reduce the
+  // overhead due to timers (see bug 1442353).
+  _getTimeoutPromise() {
+    if (this._timeoutPromise && Cu.now() <= this._timeoutPromiseExpires) {
+      return this._timeoutPromise;
+    }
+    let timeoutPromise = new Promise((resolve, reject) => {
+      setTimeout(() => {
+        // Clear out this._timeoutPromise if it hasn't changed since we set it.
+        if (this._timeoutPromise == timeoutPromise) {
+          this._timeoutPromise = null;
+        }
+        reject(new Error("Transaction timeout, most likely caused by unresolved pending work."));
+      }, TRANSACTIONS_QUEUE_TIMEOUT_MS);
+    });
+    this._timeoutPromise = timeoutPromise;
+    this._timeoutPromiseExpires = Cu.now() + TRANSACTIONS_QUEUE_TIMEOUT_MS * 0.2;
+    return this._timeoutPromise;
   }
 });
 
 /**
  * Opens a connection to a SQLite database.
  *
  * The following parameters can control the connection:
  *