Always log exceptions in rollbackMasterChangesAndLog()

MWExceptionHandler::rollbackMasterChangesAndLog() only logged exceptions
if there were already master changes. This is extremely problematic when
debugging, especially in situations like DeferredUpdates where they were
silently being swallowed.

This makes it log exceptions in all paths, erring on the side of logging
the same exception twice (theoretically it's possible I suppose) instead
of not at all.

Also make the method able to handle DBError exceptions, which most of
the callers seemed to be assuming. ApiMain was handling this explicitly.

Bug: T168347
Change-Id: I8739051f824a455ba669344184c3b11ac95cb561

diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index 5cb7967..52f79ee 100644 (file)
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -581,23 +581,12 @@ class ApiMain extends ApiBase {
                // T65145: Rollback any open database transactions
                if ( !( $e instanceof ApiUsageException || $e instanceof UsageException ) ) {
                        // UsageExceptions are intentional, so don't rollback if that's the case
-                       try {
-                               MWExceptionHandler::rollbackMasterChangesAndLog( $e );
-                       } catch ( DBError $e2 ) {
-                               // Rollback threw an exception too. Log it, but don't interrupt
-                               // our regularly scheduled exception handling.
-                               MWExceptionHandler::logException( $e2 );
-                       }
+                       MWExceptionHandler::rollbackMasterChangesAndLog( $e );
                }
 
                // Allow extra cleanup and logging
                Hooks::run( 'ApiMain::onException', [ $this, $e ] );
 
-               // Log it
-               if ( !( $e instanceof ApiUsageException || $e instanceof UsageException ) ) {
-                       MWExceptionHandler::logException( $e );
-               }
-
                // Handle any kind of exception by outputting properly formatted error message.
                // If this fails, an unhandled exception should be thrown so that global error
                // handler will process and log it.

diff --git a/includes/exception/MWExceptionHandler.php b/includes/exception/MWExceptionHandler.php
index 433274e..ef81366 100644 (file)
--- a/includes/exception/MWExceptionHandler.php
+++ b/includes/exception/MWExceptionHandler.php
@@ -83,29 +83,32 @@ 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.
+        * Roll back any open database transactions and log the stack trace of the exception
+        *
+        * This method is used to attempt to recover from exceptions
         *
         * @since 1.23
         * @param Exception|Throwable $e
         */
        public static function rollbackMasterChangesAndLog( $e ) {
                $services = MediaWikiServices::getInstance();
-               if ( $services->isServiceDisabled( 'DBLoadBalancerFactory' ) ) {
-                       return; // T147599
+               if ( !$services->isServiceDisabled( 'DBLoadBalancerFactory' ) ) {
+                       // Rollback DBs to avoid transaction notices. This might fail
+                       // to rollback some databases due to connection issues or exceptions.
+                       // However, any sane DB driver will rollback implicitly anyway.
+                       try {
+                               $services->getDBLoadBalancerFactory()->rollbackMasterChanges( __METHOD__ );
+                       } catch ( DBError $e2 ) {
+                               // If the DB is unreacheable, rollback() will throw an error
+                               // and the error report() method might need messages from the DB,
+                               // which would result in an exception loop. PHP may escalate such
+                               // errors to "Exception thrown without a stack frame" fatals, but
+                               // it's better to be explicit here.
+                               self::logException( $e2, self::CAUGHT_BY_HANDLER );
+                       }
                }
 
-               $lbFactory = $services->getDBLoadBalancerFactory();
-               if ( $lbFactory->hasMasterChanges() ) {
-                       $logger = LoggerFactory::getInstance( 'Bug56269' );
-                       $logger->warning(
-                               'Exception thrown with an uncommited database transaction: ' .
-                               self::getLogMessage( $e ),
-                               self::getLogContext( $e )
-                       );
-               }
-               $lbFactory->rollbackMasterChanges( __METHOD__ );
+               self::logException( $e, self::CAUGHT_BY_HANDLER );
        }
 
        /**
@@ -123,23 +126,8 @@ class MWExceptionHandler {
         * @param Exception|Throwable $e
         */
        public static function handleException( $e ) {
-               try {
-                       // Rollback DBs to avoid transaction notices. This may fail
-                       // to rollback some DB due to connection issues or exceptions.
-                       // However, any sane DB driver will rollback implicitly anyway.
-                       self::rollbackMasterChangesAndLog( $e );
-               } catch ( DBError $e2 ) {
-                       // If the DB is unreacheable, rollback() will throw an error
-                       // and the error report() method might need messages from the DB,
-                       // which would result in an exception loop. PHP may escalate such
-                       // errors to "Exception thrown without a stack frame" fatals, but
-                       // it's better to be explicit here.
-                       self::logException( $e2, self::CAUGHT_BY_HANDLER );
-               }
-
-               self::logException( $e, self::CAUGHT_BY_HANDLER );
+               self::rollbackMasterChangesAndLog( $e );
                self::report( $e );
-
                // Exit value should be nonzero for the benefit of shell jobs
                exit( 1 );
        }