Make Database disconnect and error suppression more robust

* Disallow $ignoreErrors in query() on deadlocks, since that would otherwise
  silently rollback all changes from any other callers.
* Move recoverability checks for disconnects to canRecoverFromDisconnect().
* The first write of a DBO_TRX transaction is now considered recoverable.
* Run onTransactionResolution() callbacks on disconnect/deadlock rollback.
  Some DeferrableUpdate need this to know to abort.
* Disallow $ignoreErrors on disconnects considered unrecoverable. This
  makes it so that query() callers cannot cause writes from other callers
  to be silently lost, which is hard to reason about.
* Moved ping() logic to simple reconnect() method and ping() simply do
  a dummy SELECT, which triggeres reconnection if safe. Previously,
  ping() might cause subtle partial transaction loss.
* Remove ping() from strencode(), which would cause partial transaction
  loss where it actually reached.
* Remove mysqlPing() per https://bugs.php.net/bug.php?id=52561.

Bug: T142079
Change-Id: Ifb7f772ae849d67c0d92240a115c3f392e252937

diff --git a/includes/db/Database.php b/includes/db/Database.php
index c591f3c..4e358d4 100644 (file)
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -783,6 +783,7 @@ abstract class DatabaseBase implements IDatabase {
        public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) {
                global $wgUser;
 
+               $priorWritesPending = $this->writesOrCallbacksPending();
                $this->mLastQuery = $sql;
 
                $isWriteQuery = $this->isWriteQuery( $sql );
@@ -810,7 +811,10 @@ abstract class DatabaseBase implements IDatabase {
                // Or, for one-word queries (like "BEGIN" or COMMIT") add it to the end (bug 42598)
                $commentedSql = preg_replace( '/\s|$/', " /* $fname $userName */ ", $sql, 1 );
 
-               if ( !$this->mTrxLevel && $this->getFlag( DBO_TRX ) && $this->isTransactableQuery( $sql ) ) {
+               # Start implicit transactions that wrap the request if DBO_TRX is enabled
+               if ( !$this->mTrxLevel && $this->getFlag( DBO_TRX )
+                       && $this->isTransactableQuery( $sql )
+               ) {
                        $this->begin( __METHOD__ . " ($fname)" );
                        $this->mTrxAutomatic = true;
                }
@@ -862,31 +866,23 @@ abstract class DatabaseBase implements IDatabase {
 
                # Try reconnecting if the connection was lost
                if ( false === $ret && $this->wasErrorReissuable() ) {
-                       # Transaction is gone; this can mean lost writes or REPEATABLE-READ snapshots
-                       $hadTrx = $this->mTrxLevel;
-                       # T127428: for non-write transactions, a disconnect and a COMMIT are similar:
-                       # neither changed data and in both cases any read snapshots are reset anyway.
-                       $isNoopCommit = ( !$this->writesOrCallbacksPending() && $sql === 'COMMIT' );
-                       # Update state tracking to reflect transaction loss
-                       $this->mTrxLevel = 0;
-                       $this->mTrxIdleCallbacks = []; // bug 65263
-                       $this->mTrxPreCommitCallbacks = []; // bug 65263
-                       wfDebug( "Connection lost, reconnecting...\n" );
-                       # Stash the last error values since ping() might clear them
+                       $recoverable = $this->canRecoverFromDisconnect( $sql, $priorWritesPending );
+                       # Stash the last error values before anything might clear them
                        $lastError = $this->lastError();
                        $lastErrno = $this->lastErrno();
-                       if ( $this->ping() ) {
+                       # Update state tracking to reflect transaction loss due to disconnection
+                       $this->handleTransactionLoss();
+                       wfDebug( "Connection lost, reconnecting...\n" );
+                       if ( $this->reconnect() ) {
                                wfDebug( "Reconnected\n" );
-                               $server = $this->getServer();
-                               $msg = __METHOD__ . ": lost connection to $server; reconnected";
+                               $msg = __METHOD__ . ": lost connection to {$this->getServer()}; reconnected";
                                wfDebugLog( 'DBPerformance', "$msg:\n" . wfBacktrace( true ) );
 
-                               if ( ( $hadTrx && !$isNoopCommit ) || $this->mNamedLocksHeld ) {
-                                       # Leave $ret as false and let an error be reported.
-                                       # Callers may catch the exception and continue to use the DB.
-                                       $this->reportQueryError( $lastError, $lastErrno, $sql, $fname, $tempIgnore );
+                               if ( !$recoverable ) {
+                                       # Callers may catch the exception and continue to use the DB
+                                       $this->reportQueryError( $lastError, $lastErrno, $sql, $fname );
                                } else {
-                                       # Should be safe to silently retry (no trx/callbacks/locks)
+                                       # Should be safe to silently retry the query
                                        $startTime = microtime( true );
                                        $ret = $this->doQuery( $commentedSql );
                                        $queryRuntime = microtime( true ) - $startTime;
@@ -900,6 +896,17 @@ abstract class DatabaseBase implements IDatabase {
                }
 
                if ( false === $ret ) {
+                       # Deadlocks cause the entire transaction to abort, not just the statement.
+                       # http://dev.mysql.com/doc/refman/5.7/en/innodb-error-handling.html
+                       # https://www.postgresql.org/docs/9.1/static/explicit-locking.html
+                       if ( $this->wasDeadlock() ) {
+                               if ( $this->explicitTrxActive() || $priorWritesPending ) {
+                                       $tempIgnore = false; // not recoverable
+                               }
+                               # Update state tracking to reflect transaction loss
+                               $this->handleTransactionLoss();
+                       }
+
                        $this->reportQueryError(
                                $this->lastError(), $this->lastErrno(), $sql, $fname, $tempIgnore );
                }
@@ -918,6 +925,40 @@ abstract class DatabaseBase implements IDatabase {
                return $res;
        }
 
+       private function canRecoverFromDisconnect( $sql, $priorWritesPending ) {
+               # Transaction dropped; this can mean lost writes, or REPEATABLE-READ snapshots.
+               # Dropped connections also mean that named locks are automatically released.
+               # Only allow error suppression in autocommit mode or when the lost transaction
+               # didn't matter anyway (aside from DBO_TRX snapshot loss).
+               if ( $this->mNamedLocksHeld ) {
+                       return false; // possible critical section violation
+               } elseif ( $sql === 'COMMIT' ) {
+                       return !$priorWritesPending; // nothing written anyway? (T127428)
+               } elseif ( $sql === 'ROLLBACK' ) {
+                       return true; // transaction lost...which is also what was requested :)
+               } elseif ( $this->explicitTrxActive() ) {
+                       return false; // don't drop atomocity
+               } elseif ( $priorWritesPending ) {
+                       return false; // prior writes lost from implicit transaction
+               }
+
+               return true;
+       }
+
+       private function handleTransactionLoss() {
+               $this->mTrxLevel = 0;
+               $this->mTrxIdleCallbacks = []; // bug 65263
+               $this->mTrxPreCommitCallbacks = []; // bug 65263
+               try {
+                       // Handle callbacks in mTrxEndCallbacks
+                       $this->runOnTransactionIdleCallbacks( self::TRIGGER_ROLLBACK );
+                       return null;
+               } catch ( Exception $e ) {
+                       // Already logged; move on...
+                       return $e;
+               }
+       }
+
        public function reportQueryError( $error, $errno, $sql, $fname, $tempIgnore = false ) {
                if ( $this->ignoreErrors() || $tempIgnore ) {
                        wfDebug( "SQL ERROR (ignored): $error\n" );
@@ -2509,6 +2550,7 @@ abstract class DatabaseBase implements IDatabase {
         *
         * @param integer $trigger IDatabase::TRIGGER_* constant
         * @since 1.20
+        * @throws Exception
         */
        public function runOnTransactionIdleCallbacks( $trigger ) {
                if ( $this->suppressPostCommitCallbacks ) {
@@ -2516,7 +2558,7 @@ abstract class DatabaseBase implements IDatabase {
                }
 
                $autoTrx = $this->getFlag( DBO_TRX ); // automatic begin() enabled?
-
+               /** @var Exception $e */
                $e = $ePrior = null; // last exception
                do { // callbacks may add callbacks :)
                        $callbacks = array_merge(
@@ -2788,11 +2830,20 @@ abstract class DatabaseBase implements IDatabase {
         */
        protected function doRollback( $fname ) {
                if ( $this->mTrxLevel ) {
-                       $this->query( 'ROLLBACK', $fname, true );
+                       # Disconnects cause rollback anyway, so ignore those errors
+                       $ignoreErrors = true;
+                       $this->query( 'ROLLBACK', $fname, $ignoreErrors );
                        $this->mTrxLevel = 0;
                }
        }
 
+       /**
+        * @return bool
+        */
+       protected function explicitTrxActive() {
+               return $this->mTrxLevel && ( $this->mTrxAtomicLevels || !$this->mTrxAutomatic );
+       }
+
        /**
         * Creates a new table with structure copied from existing table
         * Note that unlike most database abstraction functions, this function does not
@@ -2894,6 +2945,19 @@ abstract class DatabaseBase implements IDatabase {
        }
 
        public function ping() {
+               try {
+                       // This will reconnect if possible, or error out if not
+                       $this->query( "SELECT 1 AS ping", __METHOD__ );
+                       return true;
+               } catch ( DBError $e ) {
+                       return false;
+               }
+       }
+
+       /**
+        * @return bool
+        */
+       protected function reconnect() {
                # Stub. Not essential to override.
                return true;
        }

diff --git a/includes/db/DatabaseMysql.php b/includes/db/DatabaseMysql.php
index 5b15147..87330b0 100644 (file)
--- a/includes/db/DatabaseMysql.php
+++ b/includes/db/DatabaseMysql.php
@@ -201,10 +201,4 @@ class DatabaseMysql extends DatabaseMysqlBase {
 
                return mysql_real_escape_string( $s, $conn );
        }
-
-       protected function mysqlPing() {
-               $conn = $this->getBindingHandle();
-
-               return mysql_ping( $conn );
-       }
 }

diff --git a/includes/db/DatabaseMysqlBase.php b/includes/db/DatabaseMysqlBase.php
index 798815d..6380fe7 100644 (file)
--- a/includes/db/DatabaseMysqlBase.php
+++ b/includes/db/DatabaseMysqlBase.php
@@ -570,14 +570,7 @@ abstract class DatabaseMysqlBase extends Database {
         * @return string
         */
        function strencode( $s ) {
-               $sQuoted = $this->mysqlRealEscapeString( $s );
-
-               if ( $sQuoted === false ) {
-                       $this->ping();
-                       $sQuoted = $this->mysqlRealEscapeString( $s );
-               }
-
-               return $sQuoted;
+               return $this->mysqlRealEscapeString( $s );
        }
 
        /**
@@ -606,18 +599,7 @@ abstract class DatabaseMysqlBase extends Database {
                return strlen( $name ) && $name[0] == '`' && substr( $name, -1, 1 ) == '`';
        }
 
-       /**
-        * @return bool
-        */
-       function ping() {
-               $ping = $this->mysqlPing();
-               if ( $ping ) {
-                       // Connection was good or lost but reconnected...
-                       // @note: mysqlnd (php 5.6+) does not support this (PHP bug 52561)
-                       return true;
-               }
-
-               // Try a full disconnect/reconnect cycle if ping() failed
+       function reconnect() {
                $this->closeConnection();
                $this->mOpened = false;
                $this->mConn = false;
@@ -626,13 +608,6 @@ abstract class DatabaseMysqlBase extends Database {
                return true;
        }
 
-       /**
-        * Ping a server connection or reconnect if there is no connection
-        *
-        * @return bool
-        */
-       abstract protected function mysqlPing();
-
        function getLag() {
                if ( $this->getLagDetectionMethod() === 'pt-heartbeat' ) {
                        return $this->getLagFromPtHeartbeat();

diff --git a/includes/db/DatabaseMysqli.php b/includes/db/DatabaseMysqli.php
index d45805a..cb580cc 100644 (file)
--- a/includes/db/DatabaseMysqli.php
+++ b/includes/db/DatabaseMysqli.php
@@ -309,12 +309,6 @@ class DatabaseMysqli extends DatabaseMysqlBase {
                return $conn->real_escape_string( $s );
        }
 
-       protected function mysqlPing() {
-               $conn = $this->getBindingHandle();
-
-               return $conn->ping();
-       }
-
        /**
         * Give an id for the connection
         *

diff --git a/includes/jobqueue/JobRunner.php b/includes/jobqueue/JobRunner.php
index 98d7d12..a132dc5 100644 (file)
--- a/includes/jobqueue/JobRunner.php
+++ b/includes/jobqueue/JobRunner.php
@@ -534,7 +534,7 @@ class JobRunner implements LoggerAwareInterface {
                wfGetLBFactory()->forEachLB( function( LoadBalancer $lb ) use ( $fname ) {
                        $lb->forEachOpenConnection( function( IDatabase $conn ) use ( $fname ) {
                                if ( $conn->writesOrCallbacksPending() ) {
-                                       $conn->query( "SELECT 1", $fname );
+                                       $conn->ping();
                                }
                        } );
                } );

diff --git a/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php b/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php
index bb7eb79..bc7542a 100644 (file)
--- a/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php
+++ b/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php
@@ -76,9 +76,6 @@ class FakeDatabaseMysqlBase extends DatabaseMysqlBase {
        protected function mysqlFetchField( $res, $n ) {
        }
 
-       protected function mysqlPing() {
-       }
-
        protected function mysqlRealEscapeString( $s ) {
 
        }