Simplify Aphront transaction code

Summary:
In D1515, I introduced some excessively-complicated semantics for detecting
connections that are lost while transactional. These semantics cause us to
reenter establishConnection() and establish twice as many connections as we need
in the common case.

We don't need a hook there at all -- it's sufficient to throw the exception
rather than retrying the query when we encounter it. This doesn't have
reentrancy problems.

Test Plan:
  - Added some encapsulation-violating hooks and a unit test for them
  - Verified we no longer double-connect.

Reviewers: btrahan, nh

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T835

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

src/__phutil_library_map__.php

@@ -59,6 +59,7 @@ phutil_register_library_map(array(
     'AphrontKeyboardShortcutsAvailableView' => 'view/widget/keyboardshortcuts',
     'AphrontListFilterView' => 'view/layout/listfilter',
     'AphrontMySQLDatabaseConnection' => 'storage/connection/mysql',
+    'AphrontMySQLDatabaseConnectionTestCase' => 'storage/connection/mysql/__tests__',
     'AphrontNullView' => 'view/null',
     'AphrontPHPHTTPSink' => 'aphront/sink/php',
     'AphrontPageView' => 'view/page/base',
@@ -883,6 +884,7 @@ phutil_register_library_map(array(
     'AphrontKeyboardShortcutsAvailableView' => 'AphrontView',
     'AphrontListFilterView' => 'AphrontView',
     'AphrontMySQLDatabaseConnection' => 'AphrontDatabaseConnection',
+    'AphrontMySQLDatabaseConnectionTestCase' => 'PhabricatorTestCase',
     'AphrontNullView' => 'AphrontView',
     'AphrontPHPHTTPSink' => 'AphrontHTTPSink',
     'AphrontPageView' => 'AphrontView',

src/storage/connection/base/AphrontDatabaseConnection.php

@@ -23,7 +23,6 @@
 abstract class AphrontDatabaseConnection {
 
   private static $transactionStates = array();
-  private $initializingTransactionState;
 
   abstract public function getInsertID();
   abstract public function getAffectedRows();
@@ -121,9 +120,6 @@ abstract class AphrontDatabaseConnection {
    * @task xaction
    */
   public function isInsideTransaction() {
-    if ($this->initializingTransactionState) {
-      return false;
-    }
     $state = $this->getTransactionState();
     return ($state->getDepth() > 0);
   }
@@ -137,17 +133,10 @@ abstract class AphrontDatabaseConnection {
    * @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;
-
+    $key = $this->getTransactionKey();
+    if (empty(self::$transactionStates[$key])) {
+      self::$transactionStates[$key] = new AphrontDatabaseTransactionState();
+    }
     return self::$transactionStates[$key];
   }
 

src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php

@@ -24,6 +24,8 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
   private $config;
   private $connection;
 
+  private $nextError;
+
   private static $connectionCache = array();
 
   public function __construct(array $configuration) {
@@ -113,11 +115,6 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
       return;
     }
 
-    if ($this->isInsideTransaction()) {
-      throw new Exception(
-        "Connection was lost inside a transaction! Recovery is impossible.");
-    }
-
     $start = microtime(true);
 
     if (!function_exists('mysql_connect')) {
@@ -245,6 +242,10 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
 
         $profiler->endServiceCall($call_id, array());
 
+        if ($this->nextError) {
+          $result = null;
+        }
+
         if ($result) {
           $this->lastResult = $result;
           break;
@@ -252,23 +253,45 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
 
         $this->throwQueryException($this->connection);
       } catch (AphrontQueryConnectionLostException $ex) {
+        if ($this->isInsideTransaction()) {
+          // Zero out the transaction state to prevent a second exception
+          // ("program exited with open transaction") from being thrown, since
+          // we're about to throw a more relevant/useful one instead.
+          $state = $this->getTransactionState();
+          while ($state->getDepth()) {
+            $state->decreaseDepth();
+          }
+
+          // We can't close the connection before this because
+          // isInsideTransaction() and getTransactionState() depend on the
+          // connection.
+          $this->closeConnection();
+
+          throw $ex;
+        }
+
+        $this->closeConnection();
+
         if (!$retries) {
           throw $ex;
         }
-        if ($this->isInsideTransaction()) {
-          throw $ex;
-        }
+
         $class = get_class($ex);
         $message = $ex->getMessage();
         phlog("Retrying ({$retries}) after {$class}: {$message}");
-        $this->closeConnection();
       }
     }
   }
 
   private function throwQueryException($connection) {
-    $errno = mysql_errno($connection);
-    $error = mysql_error($connection);
+    if ($this->nextError) {
+      $errno = $this->nextError;
+      $error = 'Simulated error.';
+      $this->nextError = null;
+    } else {
+      $errno = mysql_errno($connection);
+      $error = mysql_error($connection);
+    }
 
     switch ($errno) {
       case 2013: // Connection Dropped
@@ -294,4 +317,13 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
     }
   }
 
+  /**
+   * Force the next query to fail with a simulated error. This should be used
+   * ONLY for unit tests.
+   */
+  public function simulateErrorOnNextQuery($error) {
+    $this->nextError = $error;
+    return $this;
+  }
+
 }

src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php

@@ -0,0 +1,53 @@
+<?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.
+ */
+
+final class AphrontMySQLDatabaseConnectionTestCase
+  extends PhabricatorTestCase {
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      // We disable this here because we're testing live MySQL connections.
+      self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK => false,
+    );
+  }
+
+  public function testConnectionFailures() {
+    $conn = id(new PhabricatorPHID())->establishConnection('r');
+
+    queryfx($conn, 'SELECT 1');
+
+    // We expect the connection to recover from a 2006 (lost connection) when
+    // outside of a transaction...
+    $conn->simulateErrorOnNextQuery(2006);
+    queryfx($conn, 'SELECT 1');
+
+    // ...but when transactional, we expect the query to throw when the
+    // connection is lost, because it indicates the transaction was aborted.
+    $conn->openTransaction();
+      $conn->simulateErrorOnNextQuery(2006);
+
+      $caught = null;
+      try {
+        queryfx($conn, 'SELECT 1');
+      } catch (AphrontQueryConnectionLostException $ex) {
+        $caught = $ex;
+      }
+      $this->assertEqual(true, $caught instanceof Exception);
+  }
+
+}

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

@@ -0,0 +1,16 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/phid/storage/phid');
+phutil_require_module('phabricator', 'infrastructure/testing/testcase');
+phutil_require_module('phabricator', 'storage/queryfx');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('AphrontMySQLDatabaseConnectionTestCase.php');