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 731d1b3..811870c 100644 (file)
--- 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 cab9316..1669830 100644 (file)
--- 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 f47a70b..39fb8ef 100644 (file)
--- 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 dca4d5a..3a3b467 100644 (file)
--- 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 dba368d..b114c75 100644 (file)
--- 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 0a86e94..9eff0a7 100644 (file)
--- 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.