Simplify transaction handling and restore read/write locks

Summary:
  - We used to have connection-level caching, so we needed getTransactionKey() to make sure there was one transaction state per real connection. We now cache in Lisk and each Connection object is guaranteed to represent a real, unique connection, so we can make this a non-static.
  - I kept the classes separate because it was a little easier, but maybe we should merge them?
  - Also track/implement read/write locking.
  - (The advantage of this over just writing LOCK IN SHARE MODE is that you can use, e.g., some Query class even if you don't have access to the queries it runs.)

Test Plan: Can you come up with a way to write unit tests for this? It seems like testing that it works requires deadlocking MySQL if the test is running in one process.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

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

src/__phutil_library_map__.php

@@ -79,6 +79,7 @@ phutil_register_library_map(array(
     'AphrontQueryConnectionException' => 'storage/exception/connection',
     'AphrontQueryConnectionLostException' => 'storage/exception/connectionlost',
     'AphrontQueryCountException' => 'storage/exception/count',
+    'AphrontQueryDeadlockException' => 'storage/exception/deadlock',
     'AphrontQueryDuplicateKeyException' => 'storage/exception/duplicatekey',
     'AphrontQueryException' => 'storage/exception/base',
     'AphrontQueryObjectMissingException' => 'storage/exception/objectmissing',
@@ -182,9 +183,9 @@ phutil_register_library_map(array(
     'ConduitAPI_slowvote_info_Method' => 'applications/conduit/method/slowvote/info',
     'ConduitAPI_user_Method' => 'applications/conduit/method/user/base',
     'ConduitAPI_user_addstatus_Method' => 'applications/conduit/method/user/addstatus',
-    'ConduitAPI_user_removestatus_Method' => 'applications/conduit/method/user/removestatus',
     'ConduitAPI_user_find_Method' => 'applications/conduit/method/user/find',
     'ConduitAPI_user_info_Method' => 'applications/conduit/method/user/info',
+    'ConduitAPI_user_removestatus_Method' => 'applications/conduit/method/user/removestatus',
     'ConduitAPI_user_whoami_Method' => 'applications/conduit/method/user/whoami',
     'ConduitException' => 'applications/conduit/protocol/exception',
     'DarkConsoleConfigPlugin' => 'aphront/console/plugin/config',
@@ -1118,6 +1119,7 @@ phutil_register_library_map(array(
     'AphrontQueryConnectionException' => 'AphrontQueryException',
     'AphrontQueryConnectionLostException' => 'AphrontQueryRecoverableException',
     'AphrontQueryCountException' => 'AphrontQueryException',
+    'AphrontQueryDeadlockException' => 'AphrontQueryRecoverableException',
     'AphrontQueryDuplicateKeyException' => 'AphrontQueryException',
     'AphrontQueryObjectMissingException' => 'AphrontQueryException',
     'AphrontQueryParameterException' => 'AphrontQueryException',
@@ -1207,9 +1209,9 @@ phutil_register_library_map(array(
     'ConduitAPI_slowvote_info_Method' => 'ConduitAPIMethod',
     'ConduitAPI_user_Method' => 'ConduitAPIMethod',
     'ConduitAPI_user_addstatus_Method' => 'ConduitAPI_user_Method',
-    'ConduitAPI_user_removestatus_Method' => 'ConduitAPI_user_Method',
     'ConduitAPI_user_find_Method' => 'ConduitAPI_user_Method',
     'ConduitAPI_user_info_Method' => 'ConduitAPI_user_Method',
+    'ConduitAPI_user_removestatus_Method' => 'ConduitAPI_user_Method',
     'ConduitAPI_user_whoami_Method' => 'ConduitAPI_user_Method',
     'DarkConsoleConfigPlugin' => 'DarkConsolePlugin',
     'DarkConsoleController' => 'PhabricatorController',

src/storage/connection/base/AphrontDatabaseConnection.php

@@ -22,13 +22,12 @@
  */
 abstract class AphrontDatabaseConnection {
 
-  private static $transactionStates = array();
+  private $transactionState;
 
   abstract public function getInsertID();
   abstract public function getAffectedRows();
   abstract public function selectAllResults();
   abstract public function executeRawQuery($raw_query);
-  abstract protected function getTransactionKey();
 
   abstract public function escapeString($string);
   abstract public function escapeColumnName($string);
@@ -133,11 +132,62 @@ abstract class AphrontDatabaseConnection {
    * @task xaction
    */
   protected function getTransactionState() {
-    $key = $this->getTransactionKey();
-    if (empty(self::$transactionStates[$key])) {
-      self::$transactionStates[$key] = new AphrontDatabaseTransactionState();
+    if (!$this->transactionState) {
+      $this->transactionState = new AphrontDatabaseTransactionState();
     }
-    return self::$transactionStates[$key];
+    return $this->transactionState;
+  }
+
+
+  /**
+   * @task xaction
+   */
+  public function beginReadLocking() {
+    $this->getTransactionState()->beginReadLocking();
+    return $this;
+  }
+
+
+  /**
+   * @task xaction
+   */
+  public function endReadLocking() {
+    $this->getTransactionState()->endReadLocking();
+    return $this;
+  }
+
+
+  /**
+   * @task xaction
+   */
+  public function isReadLocking() {
+    return $this->getTransactionState()->isReadLocking();
+  }
+
+
+  /**
+   * @task xaction
+   */
+  public function beginWriteLocking() {
+    $this->getTransactionState()->beginWriteLocking();
+    return $this;
+  }
+
+
+  /**
+   * @task xaction
+   */
+  public function endWriteLocking() {
+    $this->getTransactionState()->endWriteLocking();
+    return $this;
+  }
+
+
+  /**
+   * @task xaction
+   */
+  public function isWriteLocking() {
+    return $this->getTransactionState()->isWriteLocking();
   }
 
 }

src/storage/connection/isolated/AphrontIsolatedDatabaseConnection.php

@@ -25,8 +25,6 @@ final class AphrontIsolatedDatabaseConnection
   private $configuration;
   private static $nextInsertID;
   private $insertID;
-  private static $nextTransactionKey = 1;
-  private $transactionKey;
 
   private $transcript = array();
 
@@ -38,8 +36,6 @@ final class AphrontIsolatedDatabaseConnection
       // collisions and make them distinctive.
       self::$nextInsertID = 55555000000 + mt_rand(0, 1000);
     }
-
-    $this->transactionKey = 'iso-xaction-'.(self::$nextTransactionKey++);
   }
 
   public function escapeString($string) {
@@ -70,10 +66,6 @@ final class AphrontIsolatedDatabaseConnection
     return $this->affectedRows;
   }
 
-  protected function getTransactionKey() {
-    return $this->transactionKey;
-  }
-
   public function selectAllResults() {
     return $this->allResults;
   }

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

@@ -225,7 +225,7 @@ abstract class AphrontMySQLDatabaseConnectionBase
         throw new AphrontQueryConnectionLostException($exmsg);
       case 1213: // Deadlock
       case 1205: // Lock wait timeout exceeded
-        throw new AphrontQueryRecoverableException($exmsg);
+        throw new AphrontQueryDeadlockException($exmsg);
       case 1062: // Duplicate Key
         // NOTE: In some versions of MySQL we get a key name back here, but
         // older versions just give us a key index ("key 2") so it's not

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

@@ -12,8 +12,8 @@ phutil_require_module('phabricator', 'storage/connection/base');
 phutil_require_module('phabricator', 'storage/exception/accessdenied');
 phutil_require_module('phabricator', 'storage/exception/base');
 phutil_require_module('phabricator', 'storage/exception/connectionlost');
+phutil_require_module('phabricator', 'storage/exception/deadlock');
 phutil_require_module('phabricator', 'storage/exception/duplicatekey');
-phutil_require_module('phabricator', 'storage/exception/recoverable');
 phutil_require_module('phabricator', 'storage/exception/schema');
 
 phutil_require_module('phutil', 'error');

src/storage/connection/mysql/mysql/AphrontMySQLDatabaseConnection.php

@@ -34,10 +34,6 @@ final class AphrontMySQLDatabaseConnection
     return mysql_affected_rows($this->requireConnection());
   }
 
-  protected function getTransactionKey() {
-    return (int)$this->requireConnection();
-  }
-
   protected function connect() {
     if (!function_exists('mysql_connect')) {
       // We have to '@' the actual call since it can spew all sorts of silly

src/storage/connection/mysql/mysqli/AphrontMySQLiDatabaseConnection.php

@@ -36,10 +36,6 @@ final class AphrontMySQLiDatabaseConnection
     return $this->requireConnection()->affected_rows;
   }
 
-  protected function getTransactionKey() {
-    return spl_object_hash($this->requireConnection());
-  }
-
   protected function connect() {
     if (!class_exists('mysqli', false)) {
       throw new Exception(

src/storage/exception/deadlock/AphrontQueryDeadlockException.php

@@ -0,0 +1,23 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @group storage
+ */
+final class AphrontQueryDeadlockException
+  extends AphrontQueryRecoverableException { }

src/storage/exception/deadlock/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'storage/exception/recoverable');
+
+
+phutil_require_source('AphrontQueryDeadlockException.php');

src/storage/lisk/dao/LiskDAO.php

@@ -542,16 +542,11 @@ abstract class LiskDAO {
     $connection = $this->establishConnection('r');
 
     $lock_clause = '';
-/*
-
-    TODO: Restore this?
-
     if ($connection->isReadLocking()) {
       $lock_clause = 'FOR UPDATE';
     } else if ($connection->isWriteLocking()) {
       $lock_clause = 'LOCK IN SHARE MODE';
     }
-*/
 
     $args = func_get_args();
     $args = array_slice($args, 2);
@@ -1342,8 +1337,74 @@ abstract class LiskDAO {
   }
 
 
+  /**
+   * Begins read-locking selected rows with SELECT ... FOR UPDATE, so that
+   * other connections can not read them (this is an enormous oversimplification
+   * of FOR UPDATE semantics; consult the MySQL documentation for details). To
+   * end read locking, call @{method:endReadLocking}. For example:
+   *
+   *   $beach->openTransaction();
+   *     $beach->beginReadLocking();
+   *
+   *       $beach->reload();
+   *       $beach->setGrainsOfSand($beach->getGrainsOfSand() + 1);
+   *       $beach->save();
+   *
+   *     $beach->endReadLocking();
+   *   $beach->saveTransaction();
+   *
+   * @return this
+   * @task xaction
+   */
+  public function beginReadLocking() {
+    $this->establishConnection('w')->beginReadLocking();
+    return $this;
+  }
+
+
+  /**
+   * Ends read-locking that began at an earlier @{method:beginReadLocking} call.
+   *
+   * @return this
+   * @task xaction
+   */
+  public function endReadLocking() {
+    $this->establishConnection('w')->endReadLocking();
+    return $this;
+  }
+
+  /**
+   * Begins write-locking selected rows with SELECT ... LOCK IN SHARE MODE, so
+   * that other connections can not update or delete them (this is an
+   * oversimplification of LOCK IN SHARE MODE semantics; consult the
+   * MySQL documentation for details). To end write locking, call
+   * @{method:endWriteLocking}.
+   *
+   * @return this
+   * @task xaction
+   */
+  public function beginWriteLocking() {
+    $this->establishConnection('w')->beginWriteLocking();
+    return $this;
+  }
+
+
+  /**
+   * Ends write-locking that began at an earlier @{method:beginWriteLocking}
+   * call.
+   *
+   * @return this
+   * @task xaction
+   */
+  public function endWriteLocking() {
+    $this->establishConnection('w')->endWriteLocking();
+    return $this;
+  }
+
+
 /* -(  Isolation  )---------------------------------------------------------- */
 
+
   /**
    * @task isolate
    */

src/storage/transaction/AphrontDatabaseTransactionState.php

@@ -23,7 +23,9 @@
  */
 final class AphrontDatabaseTransactionState {
 
-  private $depth;
+  private $depth           = 0;
+  private $readLockLevel   = 0;
+  private $writeLockLevel  = 0;
 
   public function getDepth() {
     return $this->depth;
@@ -46,6 +48,40 @@ final class AphrontDatabaseTransactionState {
     return 'Aphront_Savepoint_'.$this->depth;
   }
 
+  public function beginReadLocking() {
+    $this->readLockLevel++;
+    return $this;
+  }
+
+  public function endReadLocking() {
+    if ($this->readLockLevel == 0) {
+      throw new Exception("Too many calls to endReadLocking()!");
+    }
+    $this->readLockLevel--;
+    return $this;
+  }
+
+  public function isReadLocking() {
+    return ($this->readLockLevel > 0);
+  }
+
+  public function beginWriteLocking() {
+    $this->writeLockLevel++;
+    return $this;
+  }
+
+  public function endWriteLocking() {
+    if ($this->writeLockLevel == 0) {
+      throw new Exception("Too many calls to endWriteLocking()!");
+    }
+    $this->writeLockLevel--;
+    return $this;
+  }
+
+  public function isWriteLocking() {
+    return ($this->writeLockLevel > 0);
+  }
+
   public function __destruct() {
     if ($this->depth) {
       throw new Exception(
@@ -53,6 +89,16 @@ final class AphrontDatabaseTransactionState {
         'implicitly rolled back. Calls to openTransaction() must always be '.
         'paired with a call to saveTransaction() or killTransaction().');
     }
+    if ($this->readLockLevel) {
+      throw new Exception(
+        'Process exited with an open read lock! Call to beginReadLocking() '.
+        'must always be paired with a call to endReadLocking().');
+    }
+    if ($this->writeLockLevel) {
+      throw new Exception(
+        'Process exited with an open write lock! Call to beginWriteLocking() '.
+        'must always be paired with a call to endWriteLocking().');
+    }
   }
 
 }