From dc7d342d93b12c4990f147423082ed3481ed3358 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 27 Mar 2014 12:18:38 -0400 Subject: [PATCH] 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 --- includes/api/ApiMain.php | 3 ++ includes/db/Database.php | 24 ++++++++++--- includes/db/LBFactory.php | 21 +++++++++++ includes/db/LoadBalancer.php | 43 +++++++++++++++++++++++ includes/exception/MWExceptionHandler.php | 21 +++++++++++ 5 files changed, 108 insertions(+), 4 deletions(-) diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index e1c087474f..f6557a5af5 100644 --- 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 b0794fb06d..52a3298cf2 100644 --- 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 ae105e2660..fcce8704af 100644 --- 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 a1703f0633..8aa8061069 100644 --- 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 64e8999781..8c7f05cb8d 100644 --- 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 -- 2.20.1