Move connection cache from "AphrontDatabaseConnection"-level to "LiskDAO"-level

Summary:
  - Currently, connections are responsible for connection caching. However, I want unit tests to be able to say "throw away the entire connection cache" with storage fixtures, and this is difficult/impossible when connections are responsible for the cache.
  - The only behavioral change is that previously we would use the same connection for read-mode and write-mode queries. We'll now establish two connections. No installs actually differentiate between the modes so it isn't particularly relevant what we do here. In the long term, we should probably check the "w" cache before building a new "r" connection, so transactional code which involves reads and writes works (we don't have any such code right now).

Test Plan: Loaded pages, verified only one connection was established per database. Ran unit tests.

Reviewers: btrahan, vrana, jungejason, edward

Reviewed By: vrana

CC: aran

Maniphest Tasks: T140

Differential Revision: https://secure.phabricator.com/D2342

src/applications/base/storage/lisk/PhabricatorLiskDAO.php

@@ -138,4 +138,8 @@ abstract class PhabricatorLiskDAO extends LiskDAO {
    * @task config
    */
   abstract public function getApplicationName();
+
+  protected function getConnectionNamespace() {
+    return self::$namespace.'_'.$this->getApplicationName();
+  }
 }

src/storage/connection/mysql/base/AphrontMySQLDatabaseConnectionBase.php

@@ -27,8 +27,6 @@ abstract class AphrontMySQLDatabaseConnectionBase
 
   private $nextError;
 
-  private static $connectionCache = array();
-
   abstract protected function connect();
   abstract protected function rawQuery($raw_query);
   abstract protected function fetchAssoc($result);
@@ -83,34 +81,15 @@ abstract class AphrontMySQLDatabaseConnectionBase
   private function closeConnection() {
     if ($this->connection) {
       $this->connection = null;
-      $key = $this->getConnectionCacheKey();
-      unset(self::$connectionCache[$key]);
     }
   }
 
-  private function getConnectionCacheKey() {
-    $user = $this->getConfiguration('user');
-    $host = $this->getConfiguration('host');
-    $database = $this->getConfiguration('database');
-
-    return "{$user}:{$host}:{$database}";
-  }
-
   private function establishConnection() {
-    $this->closeConnection();
-
-    $key = $this->getConnectionCacheKey();
-    if (isset(self::$connectionCache[$key])) {
-      $this->connection = self::$connectionCache[$key];
-      return;
-    }
-
     $start = microtime(true);
 
     $host = $this->getConfiguration('host');
     $database = $this->getConfiguration('database');
 
-
     $profiler = PhutilServiceProfiler::getInstance();
     $call_id = $profiler->beginServiceCall(
       array(
@@ -137,7 +116,6 @@ abstract class AphrontMySQLDatabaseConnectionBase
       }
     }
 
-    self::$connectionCache[$key] = $conn;
     $this->connection = $conn;
   }
 

src/storage/lisk/dao/LiskDAO.php

@@ -159,6 +159,7 @@
  * Assuming ##$obj##, ##$other## and ##$another## live on the same database,
  * this code will work correctly by establishing savepoints.
  *
+ * @task   conn    Managing Connections
  * @task   config  Configuring Lisk
  * @task   load    Loading Objects
  * @task   info    Examining Objects
@@ -187,7 +188,6 @@ abstract class LiskDAO {
   const IDS_PHID                    = 'ids-phid';
   const IDS_MANUAL                  = 'ids-manual';
 
-  private $__connections            = array();
   private $__dirtyFields            = array();
   private $__missingFields          = array();
   private static $processIsolationLevel = 0;
@@ -195,6 +195,8 @@ abstract class LiskDAO {
 
   private $__ephemeral = false;
 
+  private static $connections       = array();
+
   /**
    *  Build an empty object.
    *
@@ -235,9 +237,67 @@ abstract class LiskDAO {
     }
   }
 
+
+/* -(  Managing Connections  )----------------------------------------------- */
+
+
+  /**
+   * Establish a live connection to a database service. This method should
+   * return a new connection. Lisk handles connection caching and management;
+   * do not perform caching deeper in the stack.
+   *
+   * @param string Mode, either 'r' (reading) or 'w' (reading and writing).
+   * @return AphrontDatabaseConnection New database connection.
+   * @task conn
+   */
   abstract protected function establishLiveConnection($mode);
 
 
+  /**
+   * Return a namespace for this object's connections in the connection cache.
+   * Generally, the database name is appropriate. Two connections are considered
+   * equivalent if they have the same connection namespace and mode.
+   *
+   * @return string Connection namespace for cache
+   * @task conn
+   */
+  abstract protected function getConnectionNamespace();
+
+
+  /**
+   * Get an existing, cached connection for this object.
+   *
+   * @param mode Connection mode.
+   * @return AprontDatabaseConnection|null  Connection, if it exists in cache.
+   * @task conn
+   */
+  protected function getEstablishedConnection($mode) {
+    $key = $this->getConnectionNamespace().':'.$mode;
+    if (isset(self::$connections[$key])) {
+      return self::$connections[$key];
+    }
+    return null;
+  }
+
+
+  /**
+   * Store a connection in the connection cache.
+   *
+   * @param mode Connection mode.
+   * @param AphrontDatabaseConnection Connection to cache.
+   * @return this
+   * @task conn
+   */
+  protected function setEstablishedConnection(
+    $mode,
+    AphrontDatabaseConnection $connection) {
+
+    $key = $this->getConnectionNamespace().':'.$mode;
+    self::$connections[$key] = $connection;
+    return $this;
+  }
+
+
 /* -(  Configuring Lisk  )--------------------------------------------------- */
 
 
@@ -739,20 +799,26 @@ abstract class LiskDAO {
 
     if (self::shouldIsolateAllLiskEffectsToCurrentProcess()) {
       $mode = 'isolate-'.$mode;
-      if (!isset($this->__connections[$mode])) {
-        $this->__connections[$mode] = $this->establishIsolatedConnection($mode);
+
+      $connection = $this->getEstablishedConnection($mode);
+      if (!$connection) {
+        $connection = $this->establishIsolatedConnection($mode);
+        $this->setEstablishedConnection($mode, $connection);
       }
-      return $this->__connections[$mode];
+
+      return $connection;
     }
 
     // TODO There is currently no protection on 'r' queries against writing
     // or on 'w' queries against reading
 
-    if (!isset($this->__connections[$mode])) {
-      $this->__connections[$mode] = $this->establishLiveConnection($mode);
+    $connection = $this->getEstablishedConnection($mode);
+    if (!$connection) {
+      $connection = $this->establishLiveConnection($mode);
+      $this->setEstablishedConnection($mode, $connection);
     }
 
-    return $this->__connections[$mode];
+    return $connection;
   }
 
 

src/storage/lisk/dao/__tests__/LiskIsolationTestDAO.php

@@ -37,6 +37,10 @@ final class LiskIsolationTestDAO extends LiskDAO {
       "resource!");
   }
 
+  public function getConnectionNamespace() {
+    return 'test';
+  }
+
   public function getTableName() {
     return 'test';
   }