jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/362599 )
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:
Chad: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index 00f976e..97873c8 100644
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -580,22 +580,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/362599
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8739051f824a455ba669344184c3b11ac95cb561
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: REL1_29
Gerrit-Owner: Chad <ch...@wikimedia.org>
Gerrit-Reviewer: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Chad <ch...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
