From 4ac29d108cc28db979391ce545c93be4328b97f3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 7 Feb 2012 14:58:37 -0800
Subject: [PATCH] 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                |  2 +
 .../base/AphrontDatabaseConnection.php        | 19 ++-----
 .../mysql/AphrontMySQLDatabaseConnection.php  | 54 +++++++++++++++----
 ...AphrontMySQLDatabaseConnectionTestCase.php | 53 ++++++++++++++++++
 .../connection/mysql/__tests__/__init__.php   | 16 ++++++
 5 files changed, 118 insertions(+), 26 deletions(-)
 create mode 100644 src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php
 create mode 100644 src/storage/connection/mysql/__tests__/__init__.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index aa63bbc9d2..5e865c6787 100644
--- a/src/__phutil_library_map__.php
+++ b/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',
diff --git a/src/storage/connection/base/AphrontDatabaseConnection.php b/src/storage/connection/base/AphrontDatabaseConnection.php
index 823e75d0a5..62e185db13 100644
--- a/src/storage/connection/base/AphrontDatabaseConnection.php
+++ b/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];
   }
 
diff --git a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php
index de1aa92e53..311d935252 100644
--- a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php
+++ b/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;
+  }
+
 }
diff --git a/src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php b/src/storage/connection/mysql/__tests__/AphrontMySQLDatabaseConnectionTestCase.php
new file mode 100644
index 0000000000..bca29e9310
--- /dev/null
+++ b/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);
+  }
+
+}
diff --git a/src/storage/connection/mysql/__tests__/__init__.php b/src/storage/connection/mysql/__tests__/__init__.php
new file mode 100644
index 0000000000..aa3e537fbf
--- /dev/null
+++ b/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');