From 3c21f0e8e5459400caf0ea2934976e86fe5918e0 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 19 Jul 2016 13:43:17 -0700 Subject: [PATCH] Add AutoCommitUpdate class and replace some onTransactionIdle callers * This puts the complex logic here after the commit step for all DBs, making the main multi-DB transaction more likely to be atomic. * Made some cleanups to AtomicSectionUpdate and made it cancel if the transaction is rolled back as it should. * Also cleaned up some closures for PHP 5.4. Change-Id: If2f7bb6b1ba6daf1cfdc934f27c32b0b10431a3d --- autoload.php | 1 + includes/Title.php | 27 ++++++----- includes/deferred/AtomicSectionUpdate.php | 22 ++++++--- includes/deferred/AutoCommitUpdate.php | 56 +++++++++++++++++++++++ includes/filerepo/file/LocalFile.php | 45 ++++++++++-------- 5 files changed, 114 insertions(+), 37 deletions(-) create mode 100644 includes/deferred/AutoCommitUpdate.php diff --git a/autoload.php b/autoload.php index ecbc9b36c6..76a329f817 100644 --- a/autoload.php +++ b/autoload.php @@ -157,6 +157,7 @@ $wgAutoloadLocalClasses = [ 'AuthManagerSpecialPage' => __DIR__ . '/includes/specialpage/AuthManagerSpecialPage.php', 'AuthPlugin' => __DIR__ . '/includes/AuthPlugin.php', 'AuthPluginUser' => __DIR__ . '/includes/AuthPlugin.php', + 'AutoCommitUpdate' => __DIR__ . '/includes/deferred/AutoCommitUpdate.php', 'AutoLoader' => __DIR__ . '/includes/AutoLoader.php', 'AutoloadGenerator' => __DIR__ . '/includes/utils/AutoloadGenerator.php', 'Autopromote' => __DIR__ . '/includes/Autopromote.php', diff --git a/includes/Title.php b/includes/Title.php index 62f4060bab..8aa8cb7db7 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -4367,18 +4367,23 @@ class Title implements LinkTarget { return true; // avoid gap locking if we know it's not there } - $method = __METHOD__; - $dbw = wfGetDB( DB_MASTER ); $conds = $this->pageCond(); - $dbw->onTransactionIdle( function () use ( $dbw, $conds, $method, $purgeTime ) { - $dbTimestamp = $dbw->timestamp( $purgeTime ?: time() ); - $dbw->update( - 'page', - [ 'page_touched' => $dbTimestamp ], - $conds + [ 'page_touched < ' . $dbw->addQuotes( $dbTimestamp ) ], - $method - ); - } ); + DeferredUpdates::addUpdate( + new AutoCommitUpdate( + wfGetDB( DB_MASTER ), + __METHOD__, + function ( IDatabase $dbw, $fname ) use ( $conds, $purgeTime ) { + $dbTimestamp = $dbw->timestamp( $purgeTime ?: time() ); + $dbw->update( + 'page', + [ 'page_touched' => $dbTimestamp ], + $conds + [ 'page_touched < ' . $dbw->addQuotes( $dbTimestamp ) ], + $fname + ); + } + ), + DeferredUpdates::PRESEND + ); return true; } diff --git a/includes/deferred/AtomicSectionUpdate.php b/includes/deferred/AtomicSectionUpdate.php index a9921b3242..ccbd6b0eee 100644 --- a/includes/deferred/AtomicSectionUpdate.php +++ b/includes/deferred/AtomicSectionUpdate.php @@ -9,26 +9,34 @@ class AtomicSectionUpdate implements DeferrableUpdate { private $dbw; /** @var string */ private $fname; - /** @var Closure|callable */ + /** @var callable */ private $callback; /** * @param IDatabase $dbw * @param string $fname Caller name (usually __METHOD__) * @param callable $callback - * @throws InvalidArgumentException * @see IDatabase::doAtomicSection() */ - public function __construct( IDatabase $dbw, $fname, $callback ) { + public function __construct( IDatabase $dbw, $fname, callable $callback ) { $this->dbw = $dbw; $this->fname = $fname; - if ( !is_callable( $callback ) ) { - throw new InvalidArgumentException( 'Not a valid callback/closure!' ); - } $this->callback = $callback; + + if ( $this->dbw->trxLevel() ) { + $this->dbw->onTransactionResolution( [ $this, 'cancelOnRollback' ] ); + } } public function doUpdate() { - $this->dbw->doAtomicSection( $this->fname, $this->callback ); + if ( $this->callback ) { + $this->dbw->doAtomicSection( $this->fname, $this->callback ); + } + } + + public function cancelOnRollback( $trigger ) { + if ( $trigger === IDatabase::TRIGGER_ROLLBACK ) { + $this->callback = null; + } } } diff --git a/includes/deferred/AutoCommitUpdate.php b/includes/deferred/AutoCommitUpdate.php new file mode 100644 index 0000000000..ddf2bb878d --- /dev/null +++ b/includes/deferred/AutoCommitUpdate.php @@ -0,0 +1,56 @@ +dbw = $dbw; + $this->fname = $fname; + $this->callback = $callback; + + if ( $this->dbw->trxLevel() ) { + $this->dbw->onTransactionResolution( [ $this, 'cancelOnRollback' ] ); + } + } + + public function doUpdate() { + if ( !$this->callback ) { + return; + } + + $autoTrx = $this->dbw->getFlag( DBO_TRX ); + $this->dbw->clearFlag( DBO_TRX ); + try { + /** @var Exception $e */ + $e = null; + call_user_func_array( $this->callback, [ $this->dbw, $this->fname ] ); + } catch ( Exception $e ) { + } + if ( $autoTrx ) { + $this->dbw->setFlag( DBO_TRX ); + } + if ( $e ) { + throw $e; + } + } + + public function cancelOnRollback( $trigger ) { + if ( $trigger === IDatabase::TRIGGER_ROLLBACK ) { + $this->callback = null; + } + } +} diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php index cab9316388..234dbac919 100644 --- a/includes/filerepo/file/LocalFile.php +++ b/includes/filerepo/file/LocalFile.php @@ -1637,16 +1637,20 @@ class LocalFile extends File { // Purge the source and target files... $oldTitleFile = wfLocalFile( $this->title ); $newTitleFile = wfLocalFile( $target ); - // Hack: the lock()/unlock() pair is nested in a transaction so the locking is not - // tied to BEGIN/COMMIT. To avoid slow purges in the transaction, move them outside. - $this->getRepo()->getMasterDB()->onTransactionIdle( - function () use ( $oldTitleFile, $newTitleFile, $archiveNames ) { - $oldTitleFile->purgeEverything(); - foreach ( $archiveNames as $archiveName ) { - $oldTitleFile->purgeOldThumbnails( $archiveName ); + // To avoid slow purges in the transaction, move them outside... + DeferredUpdates::addUpdate( + new AutoCommitUpdate( + $this->getRepo()->getMasterDB(), + __METHOD__, + function () use ( $oldTitleFile, $newTitleFile, $archiveNames ) { + $oldTitleFile->purgeEverything(); + foreach ( $archiveNames as $archiveName ) { + $oldTitleFile->purgeOldThumbnails( $archiveName ); + } + $newTitleFile->purgeEverything(); } - $newTitleFile->purgeEverything(); - } + ), + DeferredUpdates::PRESEND ); if ( $status->isOK() ) { @@ -1682,7 +1686,7 @@ class LocalFile extends File { $this->lock(); // begin $batch->addCurrent(); - # Get old version relative paths + // Get old version relative paths $archiveNames = $batch->addOlds(); $status = $batch->execute(); $this->unlock(); // done @@ -1691,16 +1695,19 @@ class LocalFile extends File { DeferredUpdates::addUpdate( SiteStatsUpdate::factory( [ 'images' => -1 ] ) ); } - // Hack: the lock()/unlock() pair is nested in a transaction so the locking is not - // tied to BEGIN/COMMIT. To avoid slow purges in the transaction, move them outside. - $that = $this; - $this->getRepo()->getMasterDB()->onTransactionIdle( - function () use ( $that, $archiveNames ) { - $that->purgeEverything(); - foreach ( $archiveNames as $archiveName ) { - $that->purgeOldThumbnails( $archiveName ); + // To avoid slow purges in the transaction, move them outside... + DeferredUpdates::addUpdate( + new AutoCommitUpdate( + $this->getRepo()->getMasterDB(), + __METHOD__, + function () use ( $archiveNames ) { + $this->purgeEverything(); + foreach ( $archiveNames as $archiveName ) { + $this->purgeOldThumbnails( $archiveName ); + } } - } + ), + DeferredUpdates::PRESEND ); // Purge the CDN -- 2.20.1