Improve handling of uncommitted DB txns with "uncaught" exceptions

One of the causes (if not the cause) of bug 56269 is if something
manages to throw an exception that makes it to MediaWiki::run's
last-resort exception catcher while having an open database transaction:
the transaction never gets committed or rolled back, so it gets
implicitly rolled back and a warning is raised.

The API has the opposite problem in bug 63145: it catches the exception
but then does the normal DB shutdown which *commits* the transaction.
This is certainly the Wrong Thing to do.

Ideally, neither of these would ever happen because any code using
transactions would have its own try-catch that would catch any
exception, rollback the transaction, and then rethrow the exception. But
it happens anyway, so let's have both of these last-resort exception
handlers do the rollback, and also log the exception so the throwing
code has a better chance of being properly fixed.

Bug: 56269
Bug: 63145
Change-Id: I8f1da51187b281fe4afc0d5a0c49f5caf3612e92

diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index e1c0874..f6557a5 100644 (file)
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -386,6 +386,9 @@ class ApiMain extends ApiBase {
         * @param Exception $e
         */
        protected function handleException( Exception $e ) {
+               // Bug 63145: Rollback any open database transactions
+               MWExceptionHandler::rollbackMasterChangesAndLog( $e );
+
                // Allow extra cleanup and logging
                wfRunHooks( 'ApiMain::onException', array( $this, $e ) );
 

diff --git a/includes/db/Database.php b/includes/db/Database.php
index b0794fb..52a3298 100644 (file)
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -3452,7 +3452,7 @@ abstract class DatabaseBase implements IDatabase, DatabaseType {
                        );
                }
 
-               if ( $flush != 'flush' ) {
+               if ( $flush !== 'flush' ) {
                        if ( !$this->mTrxLevel ) {
                                wfWarn( "$fname: No transaction to commit, something got out of sync!" );
                        } elseif ( $this->mTrxAutomatic ) {
@@ -3494,11 +3494,27 @@ abstract class DatabaseBase implements IDatabase, DatabaseType {
         * No-op on non-transactional databases.
         *
         * @param string $fname
+        * @param string $flush Flush flag, set to 'flush' to disable warnings about
+        *   calling rollback when no transaction is in progress. This will silently
+        *   break any ongoing explicit transaction. Only set the flush flag if you
+        *   are sure that it is safe to ignore these warnings in your context.
+        * @since 1.23 Added $flush parameter
         */
-       final public function rollback( $fname = __METHOD__ ) {
-               if ( !$this->mTrxLevel ) {
-                       wfWarn( "$fname: No transaction to rollback, something got out of sync!" );
+       final public function rollback( $fname = __METHOD__, $flush = '' ) {
+               if ( $flush !== 'flush' ) {
+                       if ( !$this->mTrxLevel ) {
+                               wfWarn( "$fname: No transaction to rollback, something got out of sync!" );
+                       } elseif ( $this->mTrxAutomatic ) {
+                               wfWarn( "$fname: Explicit rollback of implicit transaction. Something may be out of sync!" );
+                       }
+               } else {
+                       if ( !$this->mTrxLevel ) {
+                               return; // nothing to do
+                       } elseif ( !$this->mTrxAutomatic ) {
+                               wfWarn( "$fname: Flushing an explicit transaction, getting out of sync!" );
+                       }
                }
+
                $this->doRollback( $fname );
                $this->mTrxIdleCallbacks = array(); // cancel
                $this->mTrxPreCommitCallbacks = array(); // cancel

diff --git a/includes/db/LBFactory.php b/includes/db/LBFactory.php
index ae105e2..fcce870 100644 (file)
--- a/includes/db/LBFactory.php
+++ b/includes/db/LBFactory.php
@@ -191,6 +191,27 @@ abstract class LBFactory {
        function commitMasterChanges() {
                $this->forEachLBCallMethod( 'commitMasterChanges' );
        }
+
+       /**
+        * Rollback changes on all master connections
+        * @since 1.23
+        */
+       function rollbackMasterChanges() {
+               $this->forEachLBCallMethod( 'rollbackMasterChanges' );
+       }
+
+       /**
+        * Detemine if any master connection has pending changes.
+        * @since 1.23
+        * @return bool
+        */
+       function hasMasterChanges() {
+               $ret = false;
+               $this->forEachLB( function ( $lb ) use ( &$ret ) {
+                       $ret = $ret || $lb->hasMasterChanges();
+               } );
+               return $ret;
+       }
 }
 
 /**

diff --git a/includes/db/LoadBalancer.php b/includes/db/LoadBalancer.php
index a1703f0..8aa8061 100644 (file)
--- a/includes/db/LoadBalancer.php
+++ b/includes/db/LoadBalancer.php
@@ -949,6 +949,49 @@ class LoadBalancer {
                }
        }
 
+       /**
+        * Issue ROLLBACK only on master, only if queries were done on connection
+        * @since 1.23
+        */
+       function rollbackMasterChanges() {
+               // Always 0, but who knows.. :)
+               $masterIndex = $this->getWriterIndex();
+               foreach ( $this->mConns as $conns2 ) {
+                       if ( empty( $conns2[$masterIndex] ) ) {
+                               continue;
+                       }
+                       /** @var DatabaseBase $conn */
+                       foreach ( $conns2[$masterIndex] as $conn ) {
+                               if ( $conn->trxLevel() && $conn->writesOrCallbacksPending() ) {
+                                       $conn->rollback( __METHOD__, 'flush' );
+                               }
+                       }
+               }
+       }
+
+       /**
+        * Determine if there are any pending changes that need to be rolled back
+        * or committed.
+        * @since 1.23
+        * @return bool
+        */
+       function hasMasterChanges() {
+               // Always 0, but who knows.. :)
+               $masterIndex = $this->getWriterIndex();
+               foreach ( $this->mConns as $conns2 ) {
+                       if ( empty( $conns2[$masterIndex] ) ) {
+                               continue;
+                       }
+                       /** @var DatabaseBase $conn */
+                       foreach ( $conns2[$masterIndex] as $conn ) {
+                               if ( $conn->trxLevel() && $conn->writesOrCallbacksPending() ) {
+                                       return true;
+                               }
+                       }
+               }
+               return false;
+       }
+
        /**
         * @param $value null
         * @return Mixed

diff --git a/includes/exception/MWExceptionHandler.php b/includes/exception/MWExceptionHandler.php
index 64e8999..8c7f05c 100644 (file)
--- a/includes/exception/MWExceptionHandler.php
+++ b/includes/exception/MWExceptionHandler.php
@@ -101,6 +101,25 @@ class MWExceptionHandler {
                }
        }
 
+       /**
+        * If there are any open database transactions, roll them back and log
+        * the stack trace of the exception that should have been caught so the
+        * transaction could be aborted properly.
+        * @since 1.23
+        * @param Exception $e
+        */
+       public static function rollbackMasterChangesAndLog( Exception $e ) {
+               $factory = wfGetLBFactory();
+               if ( $factory->hasMasterChanges() ) {
+                       wfDebugLog( 'Bug56269',
+                               'Exception thrown with an uncommited database transaction: ' .
+                                       MWExceptionHandler::getLogMessage( $e ) . "\n" .
+                                       $e->getTraceAsString()
+                       );
+                       $factory->rollbackMasterChanges();
+               }
+       }
+
        /**
         * Exception handler which simulates the appropriate catch() handling:
         *
@@ -115,6 +134,8 @@ class MWExceptionHandler {
        public static function handle( $e ) {
                global $wgFullyInitialised;
 
+               self::rollbackMasterChangesAndLog( $e );
+
                self::report( $e );
 
                // Final cleanup