Restore Lisk transactional methods

Summary:
Restores a (simplified and improved) version of Lisk transactions.

This doesn't actually use transactions anywhere yet. DifferentialRevisionEditor
is the #1 (and only?) case where we have transaction problems right now, but
sticking save() inside a transaction unconditionally will leave us holding a
transaction open for like a million years while we run Herald rules, etc. I want
to do some refactoring there separately from this diff before making it
transactional.

NOTE: @jungejason / @nh, can one of you verify these unit tests pass on
HPHP/i/vm when you get a chance? I vaguely recall there was some problem with
(int)$resource. We can land this safely without verifying that, but should check
before we start using it anywhere.

Test Plan: Ran unit tests.

Reviewers: btrahan, nh, jungejason

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T605

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

src/__phutil_library_map__.php

@@ -21,6 +21,7 @@ phutil_register_library_map(array(
     'AphrontController' => 'aphront/controller',
     'AphrontCrumbsView' => 'view/layout/crumbs',
     'AphrontDatabaseConnection' => 'storage/connection/base',
+    'AphrontDatabaseTransactionState' => 'storage/transaction',
     'AphrontDefaultApplicationConfiguration' => 'aphront/default/configuration',
     'AphrontDefaultApplicationController' => 'aphront/default/controller',
     'AphrontDialogResponse' => 'aphront/response/dialog',

src/storage/connection/base/AphrontDatabaseConnection.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -17,12 +17,13 @@
  */
 
 /**
+ * @task  xaction Transaction Management
  * @group storage
  */
 abstract class AphrontDatabaseConnection {
 
-  private static $transactionStacks             = array();
+  private static $transactionStates = array();
-  private static $transactionShutdownRegistered = false;
+  private $initializingTransactionState;
 
   abstract public function getInsertID();
   abstract public function getAffectedRows();
@@ -47,179 +48,107 @@ abstract class AphrontDatabaseConnection {
     return call_user_func_array('queryfx', $args);
   }
 
-  // TODO: Probably need to reset these when we catch a connection exception
-  // in the transaction stack.
-  protected function &getLockLevels() {
-    static $levels = array();
-    $key = $this->getTransactionKey();
-    if (!isset($levels[$key])) {
-      $levels[$key] = array(
-        'read'  => 0,
-        'write' => 0,
-      );
-    }
 
-    return $levels[$key];
+/* -(  Transaction Management  )--------------------------------------------- */
-  }
 
-  public function isReadLocking() {
-    $levels = &$this->getLockLevels();
-    return ($levels['read'] > 0);
-  }
-
-  public function isWriteLocking() {
-    $levels = &$this->getLockLevels();
-    return ($levels['write'] > 0);
-  }
-
-  public function startReadLocking() {
-    $levels = &$this->getLockLevels();
-    ++$levels['read'];
-    return $this;
-  }
-
-  public function startWriteLocking() {
-    $levels = &$this->getLockLevels();
-    ++$levels['write'];
-    return $this;
-  }
-
-  public function stopReadLocking() {
-    $levels = &$this->getLockLevels();
-    if ($levels['read'] < 1) {
-      throw new Exception('Unable to stop read locking: not read locking.');
-    }
-    --$levels['read'];
-    return $this;
-  }
-
-  public function stopWriteLocking() {
-    $levels = &$this->getLockLevels();
-    if ($levels['write'] < 1) {
-      throw new Exception('Unable to stop read locking: not write locking.');
-    }
-    --$levels['write'];
-    return $this;
-  }
-
-  protected function &getTransactionStack($key) {
-    if (!self::$transactionShutdownRegistered) {
-      self::$transactionShutdownRegistered = true;
-      register_shutdown_function(
-        array(
-          'AphrontDatabaseConnection',
-          'shutdownTransactionStacks',
-        ));
-    }
-
-    if (!isset(self::$transactionStacks[$key])) {
-      self::$transactionStacks[$key] = array();
-    }
-
-    return self::$transactionStacks[$key];
-  }
-
-  public static function shutdownTransactionStacks() {
-    foreach (self::$transactionStacks as $stack) {
-      if ($stack === false) {
-        continue;
-      }
-
-      $count = count($stack);
-      if ($count) {
-        throw new Exception(
-          'Script exited with '.$count.' open transactions! The '.
-          'transactions will be implicitly rolled back. Calls to '.
-          'openTransaction() should always be paired with a call to '.
-          'saveTransaction() or killTransaction(); you have an unpaired '.
-          'call somewhere.',
-          $count);
-      }
-    }
-  }
 
+  /**
+   * Begin a transaction, or set a savepoint if the connection is already
+   * transactional.
+   *
+   * @return this
+   * @task xaction
+   */
   public function openTransaction() {
-    $key = $this->getTransactionKey();
+    $state = $this->getTransactionState();
-    $stack = &$this->getTransactionStack($key);
+    $point = $state->getSavepointName();
-
+    $depth = $state->increaseDepth();
-    $new_transaction = !count($stack);
-
-    // TODO: At least in development, push context information instead of
-    // `true' so we can report (or, at least, guess) where unpaired
-    // transaction calls happened.
-    $stack[] = true;
-
-    end($stack);
-    $key = key($stack);
 
+    $new_transaction = ($depth == 1);
     if ($new_transaction) {
       $this->query('START TRANSACTION');
     } else {
-      $this->query('SAVEPOINT '.$this->getSavepointName($key));
+      $this->query('SAVEPOINT '.$point);
     }
+
+    return $this;
   }
 
-  public function isInsideTransaction() {
-    $key = $this->getTransactionKey();
-    $stack = &$this->getTransactionStack($key);
-    return (bool)count($stack);
-  }
 
+  /**
+   * Commit a transaction, or stage a savepoint for commit once the entire
+   * transaction completes if inside a transaction stack.
+   *
+   * @return this
+   * @task xaction
+   */
   public function saveTransaction() {
-    $key = $this->getTransactionKey();
+    $state = $this->getTransactionState();
-    $stack = &$this->getTransactionStack($key);
+    $depth = $state->decreaseDepth();
 
-    if (!count($stack)) {
+    if ($depth == 0) {
-      throw new Exception(
-        "No open transaction! Unable to save transaction, since there ".
-        "isn't one.");
-    }
-
-    array_pop($stack);
-
-    if (!count($stack)) {
       $this->query('COMMIT');
     }
+
+    return $this;
   }
 
-  public function saveTransactionUnless($cond) {
-    if ($cond) {
-      $this->killTransaction();
-    } else {
-      $this->saveTransaction();
-    }
-  }
-
-  public function saveTransactionIf($cond) {
-    $this->saveTransactionUnless(!$cond);
-  }
 
+  /**
+   * Rollback a transaction, or unstage the last savepoint if inside a
+   * transaction stack.
+   *
+   * @return this
+   */
   public function killTransaction() {
-    $key = $this->getTransactionKey();
+    $state = $this->getTransactionState();
-    $stack = &$this->getTransactionStack($key);
+    $depth = $state->decreaseDepth();
 
-    if (!count($stack)) {
+    if ($depth == 0) {
-      throw new Exception(
-        "No open transaction! Unable to kill transaction, since there ".
-        "isn't one.");
-    }
-
-    $count = count($stack);
-
-    end($stack);
-    $key = key($stack);
-    array_pop($stack);
-
-    if (!count($stack)) {
       $this->query('ROLLBACK');
     } else {
-      $this->query(
+      $this->query('ROLLBACK TO SAVEPOINT '.$state->getSavepointName());
-         'ROLLBACK TO SAVEPOINT '.$this->getSavepointName($key)
-        );
     }
+
+    return $this;
   }
 
-  protected function getSavepointName($key) {
+
-    return 'LiskSavepoint_'.$key;
+  /**
+   * Returns true if the connection is transactional.
+   *
+   * @return bool True if the connection is currently transactional.
+   * @task xaction
+   */
+  public function isInsideTransaction() {
+    if ($this->initializingTransactionState) {
+      return false;
+    }
+    $state = $this->getTransactionState();
+    return ($state->getDepth() > 0);
   }
+
+
+  /**
+   * Get the current @{class:AphrontDatabaseTransactionState} object, or create
+   * one if none exists.
+   *
+   * @return AphrontDatabaseTransactionState Current transaction state.
+   * @task xaction
+   */
+  protected function getTransactionState() {
+
+    // Establishing a connection may be required to get the transaction key,
+    // and may also perform a test for transaction state. While establishing
+    // transaction state, avoid infinite recursion.
+    $this->initializingTransactionState = true;
+      $key = $this->getTransactionKey();
+      if (empty(self::$transactionStates[$key])) {
+        self::$transactionStates[$key] = new AphrontDatabaseTransactionState();
+      }
+    $this->initializingTransactionState = false;
+
+    return self::$transactionStates[$key];
+  }
+
 }

src/storage/connection/base/__init__.php

@@ -7,6 +7,7 @@
 
 
 phutil_require_module('phabricator', 'storage/queryfx');
+phutil_require_module('phabricator', 'storage/transaction');
 
 
 phutil_require_source('AphrontDatabaseConnection.php');

src/storage/connection/isolated/AphrontIsolatedDatabaseConnection.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -24,6 +24,10 @@ class AphrontIsolatedDatabaseConnection extends AphrontDatabaseConnection {
   private $configuration;
   private static $nextInsertID;
   private $insertID;
+  private static $nextTransactionKey = 1;
+  private $transactionKey;
+
+  private $transcript = array();
 
   public function __construct(array $configuration) {
     $this->configuration = $configuration;
@@ -33,6 +37,8 @@ class AphrontIsolatedDatabaseConnection extends AphrontDatabaseConnection {
       // collisions and make them distinctive.
       self::$nextInsertID = 55555000000 + mt_rand(0, 1000);
     }
+
+    $this->transactionKey = 'iso-xaction-'.(self::$nextTransactionKey++);
   }
 
   public function escapeString($string) {
@@ -64,7 +70,7 @@ class AphrontIsolatedDatabaseConnection extends AphrontDatabaseConnection {
   }
 
   public function getTransactionKey() {
-    return 'xaction'; // TODO, probably need to stub this better.
+    return $this->transactionKey;
   }
 
   public function selectAllResults() {
@@ -74,17 +80,35 @@ class AphrontIsolatedDatabaseConnection extends AphrontDatabaseConnection {
   public function executeRawQuery($raw_query) {
 
     // NOTE: "[\s<>K]*" allows any number of (properly escaped) comments to
-    // appear prior to the INSERT/UPDATE/DELETE, since this connection escapes
+    // appear prior to the allowed keyword, since this connection escapes
     // them as "<K>" (above).
-    if (!preg_match('/^[\s<>K]*(INSERT|UPDATE|DELETE)\s*/i', $raw_query)) {
+
+    $keywords = array(
+      'INSERT',
+      'UPDATE',
+      'DELETE',
+      'START',
+      'SAVEPOINT',
+      'COMMIT',
+      'ROLLBACK',
+    );
+    $preg_keywords = array();
+    foreach ($keywords as $key => $word) {
+      $preg_keywords[] = preg_quote($word, '/');
+    }
+    $preg_keywords = implode('|', $preg_keywords);
+
+    if (!preg_match('/^[\s<>K]*('.$preg_keywords.')\s*/i', $raw_query)) {
       $doc_uri = PhabricatorEnv::getDoclink('article/Writing_Unit_Tests.html');
       throw new Exception(
-        "Database isolation currently only supports INSERT, UPDATE and DELETE ".
+        "Database isolation currently only supports some queries. For more ".
-        "queries. For more information, see <{$doc_uri}>. You are trying to ".
+        "information, see <{$doc_uri}>. You are trying to issue a query which ".
-        "issue a query which does not begin with INSERT, UPDATE or DELETE: ".
+        "does not begin with an allowed keyword ".
-        "'".$raw_query."'");
+        "(".implode(', ', $keywords)."): '".$raw_query."'");
     }
 
+    $this->transcript[] = $raw_query;
+
     // NOTE: This method is intentionally simplified for now, since we're only
     // using it to stub out inserts/updates. In the future it will probably need
     // to grow more powerful.
@@ -99,4 +123,8 @@ class AphrontIsolatedDatabaseConnection extends AphrontDatabaseConnection {
     $this->affectedRows = 1;
   }
 
+  public function getQueryTranscript() {
+    return $this->transcript;
+  }
+
 }

src/storage/connection/isolated/__tests__/AphrontIsolatedDatabaseConnectionTestCase.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -30,27 +30,14 @@ class AphrontIsolatedDatabaseConnectionTestCase
 
   public function testIsolation() {
     $conn = $this->newIsolatedConnection();
-
+    $test_phid = $this->generateTestPHID();
-    $test_phid = 'PHID-TEST-'.Filesystem::readRandomCharacters(20);
 
     queryfx(
       $conn,
       'INSERT INTO phabricator_phid.phid (phid) VALUES (%s)',
       $test_phid);
 
-    try {
+    $this->assertNoSuchPHID($test_phid);
-      $real_phid = id(new PhabricatorPHID())->loadOneWhere(
-        'phid = %s',
-        $test_phid);
-      $this->assertEqual(
-        null,
-        $real_phid,
-        'Expect fake PHID to exist only in isolation.');
-    } catch (AphrontQueryConnectionException $ex) {
-      // If we can't connect to the database, conclude that the isolated
-      // connection actually is isolated. Philosophically, this perhaps allows
-      // us to claim this test does not depend on the database?
-    }
   }
 
   public function testInsertGeneratesID() {
@@ -75,8 +62,103 @@ class AphrontIsolatedDatabaseConnectionTestCase
     queryfx($conn, 'DELETE');
   }
 
+  public function testTransactionStack() {
+    $conn = $this->newIsolatedConnection();
+    $conn->openTransaction();
+      queryfx($conn, 'INSERT');
+    $conn->saveTransaction();
+    $this->assertEqual(
+      array(
+        'START TRANSACTION',
+        'INSERT',
+        'COMMIT',
+      ),
+      $conn->getQueryTranscript());
+
+    $conn = $this->newIsolatedConnection();
+    $conn->openTransaction();
+      queryfx($conn, 'INSERT 1');
+      $conn->openTransaction();
+        queryfx($conn, 'INSERT 2');
+      $conn->killTransaction();
+      $conn->openTransaction();
+        queryfx($conn, 'INSERT 3');
+        $conn->openTransaction();
+          queryfx($conn, 'INSERT 4');
+        $conn->saveTransaction();
+      $conn->saveTransaction();
+      $conn->openTransaction();
+        queryfx($conn, 'INSERT 5');
+      $conn->killTransaction();
+      queryfx($conn, 'INSERT 6');
+    $conn->saveTransaction();
+
+    $this->assertEqual(
+      array(
+        'START TRANSACTION',
+        'INSERT 1',
+        'SAVEPOINT Aphront_Savepoint_1',
+        'INSERT 2',
+        'ROLLBACK TO SAVEPOINT Aphront_Savepoint_1',
+        'SAVEPOINT Aphront_Savepoint_1',
+        'INSERT 3',
+        'SAVEPOINT Aphront_Savepoint_2',
+        'INSERT 4',
+        'SAVEPOINT Aphront_Savepoint_1',
+        'INSERT 5',
+        'ROLLBACK TO SAVEPOINT Aphront_Savepoint_1',
+        'INSERT 6',
+        'COMMIT',
+      ),
+      $conn->getQueryTranscript());
+  }
+
+  public function testTransactionRollback() {
+    $check = array();
+
+    $phid = new PhabricatorPHID();
+    $phid->openTransaction();
+      for ($ii = 0; $ii < 3; $ii++) {
+        $test_phid = $this->generateTestPHID();
+
+        $obj = new PhabricatorPHID();
+        $obj->setPHID($test_phid);
+        $obj->setPHIDType('TEST');
+        $obj->setOwnerPHID('PHID-UNIT-!!!!');
+        $obj->save();
+
+        $check[] = $test_phid;
+      }
+    $phid->killTransaction();
+
+    foreach ($check as $test_phid) {
+      $this->assertNoSuchPHID($test_phid);
+    }
+  }
+
   private function newIsolatedConnection() {
     $config = array();
     return new AphrontIsolatedDatabaseConnection($config);
   }
+
+  private function generateTestPHID() {
+    return 'PHID-TEST-'.Filesystem::readRandomCharacters(20);
+  }
+
+  private function assertNoSuchPHID($phid) {
+    try {
+      $real_phid = id(new PhabricatorPHID())->loadOneWhere(
+        'phid = %s',
+        $phid);
+      $this->assertEqual(
+        null,
+        $real_phid,
+        'Expect fake PHID to exist only in isolation.');
+    } catch (AphrontQueryConnectionException $ex) {
+      // If we can't connect to the database, conclude that the isolated
+      // connection actually is isolated. Philosophically, this perhaps allows
+      // us to claim this test does not depend on the database?
+    }
+  }
+
 }

src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php

@@ -107,16 +107,17 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
   private function establishConnection() {
     $this->closeConnection();
 
-    $user = $this->getConfiguration('user');
-    $host = $this->getConfiguration('host');
-    $database = $this->getConfiguration('database');
-
     $key = $this->getConnectionCacheKey();
     if (isset(self::$connectionCache[$key])) {
       $this->connection = self::$connectionCache[$key];
       return;
     }
 
+    if ($this->isInsideTransaction()) {
+      throw new Exception(
+        "Connection was lost inside a transaction! Recovery is impossible.");
+    }
+
     $start = microtime(true);
 
     if (!function_exists('mysql_connect')) {
@@ -129,6 +130,11 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
         "available!");
     }
 
+    $user = $this->getConfiguration('user');
+    $host = $this->getConfiguration('host');
+    $database = $this->getConfiguration('database');
+
+
     $profiler = PhutilServiceProfiler::getInstance();
     $call_id = $profiler->beginServiceCall(
       array(

src/storage/lisk/dao/LiskDAO.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -135,12 +135,37 @@
  * loadAllWhere() returns a list of objects, while loadOneWhere() returns a
  * single object (or null).
  *
+ * = Managing Transactions =
+ *
+ * Lisk uses a transaction stack, so code does not generally need to be aware
+ * of the transactional state of objects to implement correct transaction
+ * semantics:
+ *
+ *   $obj->openTransaction();
+ *     $obj->save();
+ *     $other->save();
+ *     // ...
+ *     $other->openTransaction();
+ *       $other->save();
+ *       $another->save();
+ *     if ($some_condition) {
+ *       $other->saveTransaction();
+ *     } else {
+ *       $other->killTransaction();
+ *     }
+ *     // ...
+ *   $obj->saveTransaction();
+ *
+ * Assuming ##$obj##, ##$other## and ##$another## live on the same database,
+ * this code will work correctly by establishing savepoints.
+ *
  * @task   config  Configuring Lisk
  * @task   load    Loading Objects
  * @task   info    Examining Objects
  * @task   save    Writing Objects
  * @task   hook    Hooks and Callbacks
  * @task   util    Utilities
+ * @task   xaction Managing Transactions
  * @task   isolate Isolation for Unit Testing
  *
  * @group storage
@@ -454,11 +479,16 @@ 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);
@@ -1165,6 +1195,42 @@ abstract class LiskDAO {
   }
 
 
+/* -(  Manging Transactions  )----------------------------------------------- */
+
+
+  /**
+   * Increase transaction stack depth.
+   *
+   * @return this
+   */
+  public function openTransaction() {
+    $this->establishConnection('w')->openTransaction();
+    return $this;
+  }
+
+
+  /**
+   * Decrease transaction stack depth, saving work.
+   *
+   * @return this
+   */
+  public function saveTransaction() {
+    $this->establishConnection('w')->saveTransaction();
+    return $this;
+  }
+
+
+  /**
+   * Decrease transaction stack depth, discarding work.
+   *
+   * @return this
+   */
+  public function killTransaction() {
+    $this->establishConnection('w')->killTransaction();
+    return $this;
+  }
+
+
 /* -(  Isolation  )---------------------------------------------------------- */
 
   /**

src/storage/transaction/AphrontDatabaseTransactionState.php

@@ -0,0 +1,58 @@
+<?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.
+ */
+
+/**
+ * Represents current transaction state of a connection.
+ *
+ * @group storage
+ */
+final class AphrontDatabaseTransactionState {
+
+  private $depth;
+
+  public function getDepth() {
+    return $this->depth;
+  }
+
+  public function increaseDepth() {
+    return ++$this->depth;
+  }
+
+  public function decreaseDepth() {
+    if ($this->depth == 0) {
+      throw new Exception(
+        'Too many calls to saveTransaction() or killTransaction()!');
+    }
+
+    return --$this->depth;
+  }
+
+  public function getSavepointName() {
+    return 'Aphront_Savepoint_'.$this->depth;
+  }
+
+  public function __destruct() {
+    if ($this->depth) {
+      throw new Exception(
+        'Process exited with an open transaction! The transaction will be '.
+        'implicitly rolled back. Calls to openTransaction() must always be '.
+        'paired with a call to saveTransaction() or killTransaction().');
+    }
+  }
+
+}

src/storage/transaction/__init__.php

@@ -0,0 +1,10 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+
+phutil_require_source('AphrontDatabaseTransactionState.php');