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 @@ + 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 @@ +