From: Aaron Schulz Date: Thu, 21 Jul 2016 17:33:26 +0000 (-0700) Subject: Make revision deletion acquire file locks to avoid races X-Git-Tag: 1.31.0-rc.0~6247^2 X-Git-Url: http://git.cyclocoop.org/%24image?a=commitdiff_plain;h=d217cdf4b5403e092b3320d8ccffc415db784c77;p=lhc%2Fweb%2Fwiklou.git Make revision deletion acquire file locks to avoid races Also made RevisionListBase an Iterator to avoid ugly loops here Change-Id: I40d2d8cf63df95c59d0e1275e3ec45aff238e1cb --- diff --git a/includes/RevisionList.php b/includes/RevisionList.php index 731d1b3e5d..811870c8c0 100644 --- a/includes/RevisionList.php +++ b/includes/RevisionList.php @@ -23,7 +23,7 @@ /** * List for revision table items for a single page */ -abstract class RevisionListBase extends ContextSource { +abstract class RevisionListBase extends ContextSource implements Iterator { /** @var Title */ public $title; @@ -89,6 +89,10 @@ abstract class RevisionListBase extends ContextSource { return $this->current; } + public function rewind() { + $this->reset(); + } + /** * Get the current list item, or false if we are at the end * @return Revision @@ -107,6 +111,14 @@ abstract class RevisionListBase extends ContextSource { return $this->current; } + public function key() { + return $this->res ? $this->res->key(): 0; + } + + public function valid() { + return $this->res ? $this->res->valid() : false; + } + /** * Get the number of items in the list. * @return int diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php index cab9316388..1669830a3f 100644 --- a/includes/filerepo/file/LocalFile.php +++ b/includes/filerepo/file/LocalFile.php @@ -1906,14 +1906,36 @@ class LocalFile extends File { && strlen( serialize( $this->metadata ) ) <= self::CACHE_FIELD_MAX_LEN; } + /** + * @return Status + * @since 1.28 + */ + public function acquireFileLock() { + return $this->getRepo()->getBackend()->lockFiles( + [ $this->getPath() ], LockManager::LOCK_EX, 10 + ); + } + + /** + * @return Status + * @since 1.28 + */ + public function releaseFileLock() { + return $this->getRepo()->getBackend()->unlockFiles( + [ $this->getPath() ], LockManager::LOCK_EX + ); + } + /** * Start an atomic DB section and lock the image for update * or increments a reference counter if the lock is already held * + * This method should not be used outside of LocalFile/LocalFile*Batch + * * @throws LocalFileLockError Throws an error if the lock was not acquired * @return bool Whether the file lock owns/spawned the DB transaction */ - function lock() { + public function lock() { if ( !$this->locked ) { $logger = LoggerFactory::getInstance( 'LocalFile' ); @@ -1923,9 +1945,7 @@ class LocalFile extends File { // Bug 54736: use simple lock to handle when the file does not exist. // SELECT FOR UPDATE prevents changes, not other SELECTs with FOR UPDATE. // Also, that would cause contention on INSERT of similarly named rows. - $backend = $this->getRepo()->getBackend(); - $lockPaths = [ $this->getPath() ]; // represents all versions of the file - $status = $backend->lockFiles( $lockPaths, LockManager::LOCK_EX, 10 ); + $status = $this->acquireFileLock(); // represents all versions of the file if ( !$status->isGood() ) { $dbw->endAtomic( self::ATOMIC_SECTION_LOCK ); $logger->warning( "Failed to lock '{file}'", [ 'file' => $this->name ] ); @@ -1934,8 +1954,8 @@ class LocalFile extends File { } // Release the lock *after* commit to avoid row-level contention. // Make sure it triggers on rollback() as well as commit() (T132921). - $dbw->onTransactionResolution( function () use ( $backend, $lockPaths, $logger ) { - $status = $backend->unlockFiles( $lockPaths, LockManager::LOCK_EX ); + $dbw->onTransactionResolution( function () use ( $logger ) { + $status = $this->releaseFileLock(); if ( !$status->isGood() ) { $logger->error( "Failed to unlock '{file}'", [ 'file' => $this->name ] ); } @@ -1952,10 +1972,12 @@ class LocalFile extends File { /** * Decrement the lock reference count and end the atomic section if it reaches zero * + * This method should not be used outside of LocalFile/LocalFile*Batch + * * The commit and loc release will happen when no atomic sections are active, which * may happen immediately or at some point after calling this */ - function unlock() { + public function unlock() { if ( $this->locked ) { --$this->locked; if ( !$this->locked ) { diff --git a/includes/revisiondelete/RevDelArchivedFileItem.php b/includes/revisiondelete/RevDelArchivedFileItem.php index f47a70b562..39fb8ef6b0 100644 --- a/includes/revisiondelete/RevDelArchivedFileItem.php +++ b/includes/revisiondelete/RevDelArchivedFileItem.php @@ -23,9 +23,17 @@ * Item class for a filearchive table row */ class RevDelArchivedFileItem extends RevDelFileItem { + /** @var RevDelArchivedFileList */ + protected $list; + /** @var ArchivedFile */ + protected $file; + /** @var LocalFile */ + protected $lockFile; + public function __construct( $list, $row ) { RevDelItem::__construct( $list, $row ); $this->file = ArchivedFile::newFromRow( $row ); + $this->lockFile = RepoGroup::singleton()->getLocalRepo()->newFile( $row->fa_name ); } public function getIdField() { @@ -125,4 +133,12 @@ class RevDelArchivedFileItem extends RevDelFileItem { return $ret; } + + public function lock() { + return $this->lockFile->acquireFileLock(); + } + + public function unlock() { + return $this->lockFile->releaseFileLock(); + } } diff --git a/includes/revisiondelete/RevDelFileItem.php b/includes/revisiondelete/RevDelFileItem.php index dca4d5a6bf..3a3b46776a 100644 --- a/includes/revisiondelete/RevDelFileItem.php +++ b/includes/revisiondelete/RevDelFileItem.php @@ -23,8 +23,10 @@ * Item class for an oldimage table row */ class RevDelFileItem extends RevDelItem { - /** @var File */ + /** @var OldLocalFile */ public $file; + /** @var RevDelFileList */ + protected $list; public function __construct( $list, $row ) { parent::__construct( $list, $row ); @@ -233,4 +235,12 @@ class RevDelFileItem extends RevDelItem { return $ret; } + + public function lock() { + return $this->file->acquireFileLock(); + } + + public function unlock() { + return $this->file->releaseFileLock(); + } } diff --git a/includes/revisiondelete/RevDelItem.php b/includes/revisiondelete/RevDelItem.php index dba368d849..b114c7520c 100644 --- a/includes/revisiondelete/RevDelItem.php +++ b/includes/revisiondelete/RevDelItem.php @@ -61,4 +61,22 @@ abstract class RevDelItem extends RevisionItemBase { * @return array Data for the API result */ abstract public function getApiData( ApiResult $result ); + + /** + * Lock the item against changes outside of the DB + * @return Status + * @since 1.28 + */ + public function lock() { + return Status::newGood(); + } + + /** + * Unlock the item against changes outside of the DB + * @return Status + * @since 1.28 + */ + public function unlock() { + return Status::newGood(); + } } diff --git a/includes/revisiondelete/RevDelList.php b/includes/revisiondelete/RevDelList.php index 0a86e943d8..9eff0a70c1 100644 --- a/includes/revisiondelete/RevDelList.php +++ b/includes/revisiondelete/RevDelList.php @@ -81,14 +81,13 @@ abstract class RevDelList extends RevisionListBase { public function areAnySuppressed() { $bit = $this->getSuppressBit(); - // @codingStandardsIgnoreStart Generic.CodeAnalysis.ForLoopWithTestFunctionCall.NotAllowed - for ( $this->reset(); $this->current(); $this->next() ) { - // @codingStandardsIgnoreEnd - $item = $this->current(); + /** @var $item RevDelItem */ + foreach ( $this as $item ) { if ( $item->getBits() & $bit ) { return true; } } + return false; } @@ -104,6 +103,8 @@ abstract class RevDelList extends RevisionListBase { * @since 1.23 Added 'perItemStatus' param */ public function setVisibility( array $params ) { + $status = Status::newGood(); + $bitPars = $params['value']; $comment = $params['comment']; $perItemStatus = isset( $params['perItemStatus'] ) ? $params['perItemStatus'] : false; @@ -113,9 +114,17 @@ abstract class RevDelList extends RevisionListBase { $dbw = wfGetDB( DB_MASTER ); $this->res = $this->doQuery( $dbw ); + $status->merge( $this->acquireItemLocks() ); + if ( !$status->isGood() ) { + return $status; + } + $dbw->startAtomic( __METHOD__ ); + $dbw->onTransactionResolution( function () { + // Release locks on commit or error + $this->releaseItemLocks(); + } ); - $status = Status::newGood(); $missing = array_flip( $this->ids ); $this->clearFileOps(); $idsForLog = []; @@ -136,11 +145,8 @@ abstract class RevDelList extends RevisionListBase { // passed to doPostCommitUpdates(). $visibilityChangeMap = []; - // @codingStandardsIgnoreStart Generic.CodeAnalysis.ForLoopWithTestFunctionCall.NotAllowed - for ( $this->reset(); $this->current(); $this->next() ) { - // @codingStandardsIgnoreEnd - /** @var $item RevDelItem */ - $item = $this->current(); + /** @var $item RevDelItem */ + foreach ( $this as $item ) { unset( $missing[$item->getId()] ); if ( $perItemStatus ) { @@ -275,6 +281,26 @@ abstract class RevDelList extends RevisionListBase { return $status; } + final protected function acquireItemLocks() { + $status = Status::newGood(); + /** @var $item RevDelItem */ + foreach ( $this as $item ) { + $status->merge( $item->lock() ); + } + + return $status; + } + + final protected function releaseItemLocks() { + $status = Status::newGood(); + /** @var $item RevDelItem */ + foreach ( $this as $item ) { + $status->merge( $item->unlock() ); + } + + return $status; + } + /** * Reload the list data from the master DB. This can be done after setVisibility() * to allow $item->getHTML() to show the new data.