jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/361780 )
Change subject: Always log exceptions in rollbackMasterChangesAndLog() ...................................................................... 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 --- M includes/api/ApiMain.php M includes/exception/MWExceptionHandler.php 2 files changed, 20 insertions(+), 43 deletions(-) Approvals: Krinkle: Looks good to me, but someone else must approve Chad: Looks good to me, approved Legoktm: Looks good to me, but someone else must approve jenkins-bot: Verified diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index 5cb7967..52f79ee 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -581,22 +581,11 @@ // 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 diff --git a/includes/exception/MWExceptionHandler.php b/includes/exception/MWExceptionHandler.php index 433274e..ef81366 100644 --- a/includes/exception/MWExceptionHandler.php +++ b/includes/exception/MWExceptionHandler.php @@ -83,29 +83,32 @@ } /** - * 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 @@ * @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 ); } -- To view, visit https://gerrit.wikimedia.org/r/361780 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I8739051f824a455ba669344184c3b11ac95cb561 Gerrit-PatchSet: 4 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Legoktm <lego...@member.fsf.org> Gerrit-Reviewer: Aaron Schulz <asch...@wikimedia.org> Gerrit-Reviewer: Anomie <bjor...@wikimedia.org> Gerrit-Reviewer: Chad <ch...@wikimedia.org> Gerrit-Reviewer: Krinkle <krinklem...@gmail.com> Gerrit-Reviewer: Legoktm <lego...@member.fsf.org> Gerrit-Reviewer: Seb35 <se...@seb35.fr> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits