[FileRepo] Fixed file move data-loss race condition.
[lhc/web/wiklou.git] / includes / filerepo / file / LocalFile.php
index 467bf1b..673bf8e 100644 (file)
@@ -263,6 +263,10 @@ class LocalFile extends File {
                $this->setProps( $props );
        }
 
+       /**
+        * @param $prefix string
+        * @return array
+        */
        function getCacheFields( $prefix = 'img_' ) {
                static $fields = array( 'size', 'width', 'height', 'bits', 'media_type',
                        'major_mime', 'minor_mime', 'metadata', 'timestamp', 'sha1', 'user', 'user_text', 'description' );
@@ -311,6 +315,9 @@ class LocalFile extends File {
        /**
         * Decode a row from the database (either object or array) to an array
         * with timestamps and MIME types decoded, and the field prefix removed.
+        * @param $row
+        * @param $prefix string
+        * @throws MWException
         * @return array
         */
        function decodeRow( $row, $prefix = 'img_' ) {
@@ -433,6 +440,7 @@ class LocalFile extends File {
 
                $dbw->update( 'image',
                        array(
+                               'img_size'       => $this->size, // sanity
                                'img_width'      => $this->width,
                                'img_height'     => $this->height,
                                'img_bits'       => $this->bits,
@@ -488,6 +496,9 @@ class LocalFile extends File {
        /** getPath inherited */
        /** isVisible inhereted */
 
+       /**
+        * @return bool
+        */
        function isMissing() {
                if ( $this->missing === null ) {
                        list( $fileExists ) = $this->repo->fileExists( $this->getVirtualUrl() );
@@ -499,8 +510,8 @@ class LocalFile extends File {
        /**
         * Return the width of the image
         *
-        * Returns false on error
-        * @return bool
+        * @param $page int
+        * @return bool|int Returns false on error
         */
        public function getWidth( $page = 1 ) {
                $this->load();
@@ -520,8 +531,8 @@ class LocalFile extends File {
        /**
         * Return the height of the image
         *
-        * Returns false on error
-        * @return bool
+        * @param $page int
+        * @return bool|int Returns false on error
         */
        public function getHeight( $page = 1 ) {
                $this->load();
@@ -542,6 +553,7 @@ class LocalFile extends File {
         * Returns ID or name of user who uploaded the file
         *
         * @param $type string 'text' or 'id'
+        * @return int|string
         */
        function getUser( $type = 'text' ) {
                $this->load();
@@ -562,6 +574,9 @@ class LocalFile extends File {
                return $this->metadata;
        }
 
+       /**
+        * @return int
+        */
        function getBitDepth() {
                $this->load();
                return $this->bits;
@@ -569,6 +584,7 @@ class LocalFile extends File {
 
        /**
         * Return the size of the image file, in bytes
+        * @return int
         */
        public function getSize() {
                $this->load();
@@ -577,6 +593,7 @@ class LocalFile extends File {
 
        /**
         * Returns the mime type of the file.
+        * @return string
         */
        function getMimeType() {
                $this->load();
@@ -586,6 +603,7 @@ class LocalFile extends File {
        /**
         * Return the type of the media in the file.
         * Use the value returned by this function with the MEDIATYPE_xxx constants.
+        * @return string
         */
        function getMediaType() {
                $this->load();
@@ -615,6 +633,9 @@ class LocalFile extends File {
 
        /**
         * Fix thumbnail files from 1.4 or before, with extreme prejudice
+        * @TODO: do we still care about this? Perhaps a maintenance script
+        *        can be made instead. Enabling this code results in a serious
+        *        RTT regression for wikis without 404 handling.
         */
        function migrateThumbFile( $thumbName ) {
                $thumbDir = $this->getThumbPath();
@@ -637,10 +658,12 @@ class LocalFile extends File {
                }
                */
 
+               /*
                if ( $this->repo->fileExists( $thumbDir ) ) {
                        // Delete file where directory should be
                        $this->repo->cleanupBatch( array( $thumbDir ) );
                }
+               */
        }
 
        /** getHandler inherited */
@@ -653,8 +676,6 @@ class LocalFile extends File {
         * @return array first element is the base dir, then files in that base dir.
         */
        function getThumbnails( $archiveName = false ) {
-               $this->load();
-
                if ( $archiveName ) {
                        $dir = $this->getArchiveThumbPath( $archiveName );
                } else {
@@ -719,6 +740,8 @@ class LocalFile extends File {
         */
        function purgeOldThumbnails( $archiveName ) {
                global $wgUseSquid;
+               wfProfileIn( __METHOD__ );
+
                // Get a list of old thumbnails and URLs
                $files = $this->getThumbnails( $archiveName );
                $dir = array_shift( $files );
@@ -735,6 +758,8 @@ class LocalFile extends File {
                        }
                        SquidUpdate::purge( $urls );
                }
+
+               wfProfileOut( __METHOD__ );
        }
 
        /**
@@ -742,6 +767,7 @@ class LocalFile extends File {
         */
        function purgeThumbnails( $options = array() ) {
                global $wgUseSquid;
+               wfProfileIn( __METHOD__ );
 
                // Delete thumbnails
                $files = $this->getThumbnails();
@@ -768,6 +794,8 @@ class LocalFile extends File {
                        }
                        SquidUpdate::purge( $urls );
                }
+
+               wfProfileOut( __METHOD__ );
        }
 
        /**
@@ -800,6 +828,13 @@ class LocalFile extends File {
        /** purgeDescription inherited */
        /** purgeEverything inherited */
 
+       /**
+        * @param $limit null
+        * @param $start null
+        * @param $end null
+        * @param $inc bool
+        * @return array
+        */
        function getHistory( $limit = null, $start = null, $end = null, $inc = true ) {
                $dbr = $this->repo->getSlaveDB();
                $tables = array( 'oldimage' );
@@ -918,12 +953,12 @@ class LocalFile extends File {
         * @param $comment String: upload description
         * @param $pageText String: text to use for the new description page,
         *                  if a new description page is created
-        * @param $flags Integer: flags for publish()
-        * @param $props Array: File properties, if known. This can be used to reduce the
+        * @param $flags Integer|bool: flags for publish()
+        * @param $props Array|bool: File properties, if known. This can be used to reduce the
         *               upload time when uploading virtual URLs for which the file info
         *               is already known
-        * @param $timestamp String: timestamp for img_timestamp, or false to use the current time
-        * @param $user Mixed: User object or null to use $wgUser
+        * @param $timestamp String|bool: timestamp for img_timestamp, or false to use the current time
+        * @param $user User|null: User object or null to use $wgUser
         *
         * @return FileRepoStatus object. On success, the value member contains the
         *     archive name, or an empty string if it was a new file.
@@ -957,6 +992,13 @@ class LocalFile extends File {
 
        /**
         * Record a file upload in the upload log and the image table
+        * @param $oldver
+        * @param $desc string
+        * @param $license string
+        * @param $copyStatus string
+        * @param $source string
+        * @param $watch bool
+        * @param $timestamp string|bool
         * @return bool
         */
        function recordUpload( $oldver, $desc, $license = '', $copyStatus = '', $source = '',
@@ -977,11 +1019,19 @@ class LocalFile extends File {
 
        /**
         * Record a file upload in the upload log and the image table
+        * @param $oldver
+        * @param $comment string
+        * @param $pageText string
+        * @param $props bool|array
+        * @param $timestamp bool|string
+        * @param $user null|User
         * @return bool
         */
        function recordUpload2(
                $oldver, $comment, $pageText, $props = false, $timestamp = false, $user = null
        ) {
+               wfProfileIn( __METHOD__ );
+
                if ( is_null( $user ) ) {
                        global $wgUser;
                        $user = $wgUser;
@@ -991,7 +1041,9 @@ class LocalFile extends File {
                $dbw->begin( __METHOD__ );
 
                if ( !$props ) {
+                       wfProfileIn( __METHOD__ . '-getProps' );
                        $props = $this->repo->getFileProps( $this->getVirtualUrl() );
+                       wfProfileOut( __METHOD__ . -'getProps' );
                }
 
                if ( $timestamp === false ) {
@@ -1004,15 +1056,10 @@ class LocalFile extends File {
                $props['timestamp'] = wfTimestamp( TS_MW, $timestamp ); // DB -> TS_MW
                $this->setProps( $props );
 
-               # Delete thumbnails
-               $this->purgeThumbnails();
-
-               # The file is already on its final location, remove it from the squid cache
-               SquidUpdate::purge( array( $this->getURL() ) );
-
                # Fail now if the file isn't there
                if ( !$this->fileExists ) {
                        wfDebug( __METHOD__ . ": File " . $this->getRel() . " went missing!\n" );
+                       wfProfileOut( __METHOD__ );
                        return false;
                }
 
@@ -1041,7 +1088,6 @@ class LocalFile extends File {
                        __METHOD__,
                        'IGNORE'
                );
-
                if ( $dbw->affectedRows() == 0 ) {
                        # (bug 34993) Note: $oldver can be empty here, if the previous
                        # version of the file was broken. Allow registration of the new
@@ -1094,16 +1140,8 @@ class LocalFile extends File {
                                __METHOD__
                        );
                } else {
-                       # This is a new file
-                       # Update the image count
-                       $dbw->begin( __METHOD__ );
-                       $dbw->update(
-                               'site_stats',
-                               array( 'ss_images = ss_images+1' ),
-                               '*',
-                               __METHOD__
-                       );
-                       $dbw->commit( __METHOD__ );
+                       # This is a new file, so update the image count
+                       DeferredUpdates::addUpdate( SiteStatsUpdate::factory( array( 'images' => 1 ) ) );
                }
 
                $descTitle = $this->getTitle();
@@ -1115,6 +1153,7 @@ class LocalFile extends File {
                $action = $reupload ? 'overwrite' : 'upload';
                $log->addEntry( $action, $descTitle, $comment, array(), $user );
 
+               wfProfileIn( __METHOD__ . '-edit' );
                if ( $descTitle->exists() ) {
                        # Create a null revision
                        $latest = $descTitle->getLatestRevID();
@@ -1139,6 +1178,7 @@ class LocalFile extends File {
                        # Squid and file cache for the description page are purged by doEdit.
                        $wikiPage->doEdit( $pageText, $comment, EDIT_NEW | EDIT_SUPPRESS_RC, false, $user );
                }
+               wfProfileOut( __METHOD__ . '-edit' );
 
                # Commit the transaction now, in case something goes wrong later
                # The most important thing is that files don't get lost, especially archives
@@ -1150,8 +1190,20 @@ class LocalFile extends File {
                # which in fact doesn't really exist (bug 24978)
                $this->saveToCache();
 
+               if ( $reupload ) {
+                       # Delete old thumbnails
+                       wfProfileIn( __METHOD__ . '-purge' );
+                       $this->purgeThumbnails();
+                       wfProfileOut( __METHOD__ . '-purge' );
+
+                       # Remove the old file from the squid cache
+                       SquidUpdate::purge( array( $this->getURL() ) );
+               }
+
                # Hooks, hooks, the magic of hooks...
+               wfProfileIn( __METHOD__ . '-hooks' );
                wfRunHooks( 'FileUpload', array( $this, $reupload, $descTitle->exists() ) );
+               wfProfileOut( __METHOD__ . '-hooks' );
 
                # Invalidate cache for all pages using this file
                $update = new HTMLCacheUpdate( $this->getTitle(), 'imagelinks' );
@@ -1165,6 +1217,7 @@ class LocalFile extends File {
                        $update->doUpdate();
                }
 
+               wfProfileOut( __METHOD__ );
                return true;
        }
 
@@ -1250,13 +1303,16 @@ class LocalFile extends File {
 
                $this->lock(); // begin
                $batch->addCurrent();
-               $batch->addOlds();
+               $archiveNames = $batch->addOlds();
                $status = $batch->execute();
                $this->unlock(); // done
 
                wfDebugLog( 'imagemove', "Finished moving {$this->name}" );
 
                $this->purgeEverything();
+               foreach ( $archiveNames as $archiveName ) {
+                       $this->purgeOldThumbnails( $archiveName );
+               }
                if ( $status->isOK() ) {
                        // Now switch the object
                        $this->title = $target;
@@ -1287,27 +1343,18 @@ class LocalFile extends File {
                        return $this->readOnlyFatalStatus();
                }
 
-               $dbw = $this->repo->getMasterDB();
                $batch = new LocalFileDeleteBatch( $this, $reason, $suppress );
-               $archiveNames = array();
 
                $this->lock(); // begin
                $batch->addCurrent();
                # Get old version relative paths
-               $result = $dbw->select( 'oldimage',
-                       array( 'oi_archive_name' ),
-                       array( 'oi_name' => $this->getName() ) );
-               foreach ( $result as $row ) {
-                       $batch->addOld( $row->oi_archive_name );
-                       $archiveNames[] = $row->oi_archive_name;
-               }
+               $archiveNames = $batch->addOlds();
                $status = $batch->execute();
+               $this->unlock(); // done
+
                if ( $status->isOK() ) {
-                       // Update site_stats
-                       $site_stats = $dbw->tableName( 'site_stats' );
-                       $dbw->query( "UPDATE $site_stats SET ss_images=ss_images-1", __METHOD__ );
+                       DeferredUpdates::addUpdate( SiteStatsUpdate::factory( array( 'images' => -1 ) ) );
                }
-               $this->unlock(); // done
 
                $this->purgeEverything();
                foreach ( $archiveNames as $archiveName ) {
@@ -1336,15 +1383,14 @@ class LocalFile extends File {
                        return $this->readOnlyFatalStatus();
                }
 
-               $this->lock(); // begin
-
                $batch = new LocalFileDeleteBatch( $this, $reason, $suppress );
+
+               $this->lock(); // begin
                $batch->addOld( $archiveName );
-               $this->purgeOldThumbnails( $archiveName );
                $status = $batch->execute();
-
                $this->unlock(); // done
 
+               $this->purgeOldThumbnails( $archiveName );
                if ( $status->isOK() ) {
                        $this->purgeDescription();
                        $this->purgeHistory();
@@ -1418,16 +1464,25 @@ class LocalFile extends File {
                return $pout->getText();
        }
 
+       /**
+        * @return string
+        */
        function getDescription() {
                $this->load();
                return $this->description;
        }
 
+       /**
+        * @return bool|string
+        */
        function getTimestamp() {
                $this->load();
                return $this->timestamp;
        }
 
+       /**
+        * @return string
+        */
        function getSha1() {
                $this->load();
                // Initialise now if necessary
@@ -1450,6 +1505,9 @@ class LocalFile extends File {
                return $this->sha1;
        }
 
+       /**
+        * @return bool
+        */
        function isCacheable() {
                $this->load();
                return strlen( $this->metadata ) <= self::CACHE_FIELD_MAX_LEN; // avoid OOMs
@@ -1520,6 +1578,11 @@ class LocalFileDeleteBatch {
        var $reason, $srcRels = array(), $archiveUrls = array(), $deletionBatch, $suppress;
        var $status;
 
+       /**
+        * @param $file File
+        * @param $reason string
+        * @param $suppress bool
+        */
        function __construct( File $file, $reason = '', $suppress = false ) {
                $this->file = $file;
                $this->reason = $reason;
@@ -1531,11 +1594,39 @@ class LocalFileDeleteBatch {
                $this->srcRels['.'] = $this->file->getRel();
        }
 
+       /**
+        * @param $oldName string
+        */
        function addOld( $oldName ) {
                $this->srcRels[$oldName] = $this->file->getArchiveRel( $oldName );
                $this->archiveUrls[] = $this->file->getArchiveUrl( $oldName );
        }
 
+       /**
+        * Add the old versions of the image to the batch
+        * @return Array List of archive names from old versions
+        */
+       function addOlds() {
+               $archiveNames = array();
+
+               $dbw = $this->file->repo->getMasterDB();
+               $result = $dbw->select( 'oldimage',
+                       array( 'oi_archive_name' ),
+                       array( 'oi_name' => $this->file->getName() ),
+                       __METHOD__
+               );
+
+               foreach ( $result as $row ) {
+                       $this->addOld( $row->oi_archive_name );
+                       $archiveNames[] = $row->oi_archive_name;
+               }
+
+               return $archiveNames;
+       }
+
+       /**
+        * @return array
+        */
        function getOldRels() {
                if ( !isset( $this->srcRels['.'] ) ) {
                        $oldRels =& $this->srcRels;
@@ -1549,6 +1640,9 @@ class LocalFileDeleteBatch {
                return array( $oldRels, $deleteCurrent );
        }
 
+       /**
+        * @return array
+        */
        protected function getHashes() {
                $hashes = array();
                list( $oldRels, $deleteCurrent ) = $this->getOldRels();
@@ -1712,7 +1806,6 @@ class LocalFileDeleteBatch {
         * @return FileRepoStatus
         */
        function execute() {
-               global $wgUseSquid;
                wfProfileIn( __METHOD__ );
 
                $this->file->lock();
@@ -1777,17 +1870,6 @@ class LocalFileDeleteBatch {
                        return $this->status;
                }
 
-               // Purge squid
-               if ( $wgUseSquid ) {
-                       $urls = array();
-
-                       foreach ( $this->srcRels as $srcRel ) {
-                               $urlRel = str_replace( '%2F', '/', rawurlencode( $srcRel ) );
-                               $urls[] = $this->file->repo->getZoneUrl( 'public' ) . '/' . $urlRel;
-                       }
-                       SquidUpdate::purge( $urls );
-               }
-
                // Delete image/oldimage rows
                $this->doDBDeletes();
 
@@ -1800,6 +1882,7 @@ class LocalFileDeleteBatch {
 
        /**
         * Removes non-existent files from a deletion batch.
+        * @param $batch array
         * @return array
         */
        function removeNonexistentFiles( $batch ) {
@@ -1836,6 +1919,10 @@ class LocalFileRestoreBatch {
 
        var $cleanupBatch, $ids, $all, $unsuppress = false;
 
+       /**
+        * @param $file File
+        * @param $unsuppress bool
+        */
        function __construct( File $file, $unsuppress = false ) {
                $this->file = $file;
                $this->cleanupBatch = $this->ids = array();
@@ -2074,9 +2161,7 @@ class LocalFileRestoreBatch {
                        if ( !$exists ) {
                                wfDebug( __METHOD__ . " restored {$status->successCount} items, creating a new current\n" );
 
-                               // Update site_stats
-                               $site_stats = $dbw->tableName( 'site_stats' );
-                               $dbw->query( "UPDATE $site_stats SET ss_images=ss_images+1", __METHOD__ );
+                               DeferredUpdates::addUpdate( SiteStatsUpdate::factory( array( 'images' => 1 ) ) );
 
                                $this->file->purgeEverything();
                        } else {
@@ -2093,12 +2178,14 @@ class LocalFileRestoreBatch {
 
        /**
         * Removes non-existent files from a store batch.
+        * @param $triplets array
         * @return array
         */
        function removeNonexistentFiles( $triplets ) {
                $files = $filteredTriplets = array();
-               foreach ( $triplets as $file )
+               foreach ( $triplets as $file ) {
                        $files[$file[0]] = $file[0];
+               }
 
                $result = $this->file->repo->fileExistsBatch( $files );
 
@@ -2113,6 +2200,7 @@ class LocalFileRestoreBatch {
 
        /**
         * Removes non-existent files from a cleanup batch.
+        * @param $batch array
         * @return array
         */
        function removeNonexistentFromCleanup( $batch ) {
@@ -2192,8 +2280,17 @@ class LocalFileMoveBatch {
         */
        var $target;
 
-       var $cur, $olds, $oldCount, $archive, $db;
+       var $cur, $olds, $oldCount, $archive;
+
+       /**
+        * @var DatabaseBase
+        */
+       var $db;
 
+       /**
+        * @param File $file
+        * @param Title $target
+        */
        function __construct( File $file, Title $target ) {
                $this->file = $file;
                $this->target = $target;
@@ -2203,7 +2300,7 @@ class LocalFileMoveBatch {
                $this->newName = $this->file->repo->getNameFromTitle( $this->target );
                $this->oldRel = $this->oldHash . $this->oldName;
                $this->newRel = $this->newHash . $this->newName;
-               $this->db = $file->repo->getMasterDb();
+               $this->db = $file->getRepo()->getMasterDb();
        }
 
        /**
@@ -2215,11 +2312,13 @@ class LocalFileMoveBatch {
 
        /**
         * Add the old versions of the image to the batch
+        * @return Array List of archive names from old versions
         */
        function addOlds() {
                $archiveBase = 'archive';
                $this->olds = array();
                $this->oldCount = 0;
+               $archiveNames = array();
 
                $result = $this->db->select( 'oldimage',
                        array( 'oi_archive_name', 'oi_deleted' ),
@@ -2228,6 +2327,7 @@ class LocalFileMoveBatch {
                );
 
                foreach ( $result as $row ) {
+                       $archiveNames[] = $row->oi_archive_name;
                        $oldName = $row->oi_archive_name;
                        $bits = explode( '!', $oldName, 2 );
 
@@ -2255,6 +2355,8 @@ class LocalFileMoveBatch {
                                "{$archiveBase}/{$this->newHash}{$timestamp}!{$this->newName}"
                        );
                }
+
+               return $archiveNames;
        }
 
        /**
@@ -2264,30 +2366,37 @@ class LocalFileMoveBatch {
        function execute() {
                $repo = $this->file->repo;
                $status = $repo->newGood();
-               $triplets = $this->getMoveTriplets();
 
+               $triplets = $this->getMoveTriplets();
                $triplets = $this->removeNonexistentFiles( $triplets );
 
-               // Copy the files into their new location
-               $statusMove = $repo->storeBatch( $triplets );
-               wfDebugLog( 'imagemove', "Moved files for {$this->file->getName()}: {$statusMove->successCount} successes, {$statusMove->failCount} failures" );
-               if ( !$statusMove->isGood() ) {
-                       wfDebugLog( 'imagemove', "Error in moving files: " . $statusMove->getWikiText() );
-                       $this->cleanupTarget( $triplets );
-                       $statusMove->ok = false;
-                       return $statusMove;
-               }
-
                $this->file->lock(); // begin
+               // Rename the file versions metadata in the DB.
+               // This implicitly locks the destination file, which avoids race conditions.
+               // If we moved the files from A -> C before DB updates, another process could
+               // move files from B -> C at this point, causing storeBatch() to fail and thus
+               // cleanupTarget() to trigger. It would delete the C files and cause data loss.
                $statusDb = $this->doDBUpdates();
-               wfDebugLog( 'imagemove', "Renamed {$this->file->getName()} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
                if ( !$statusDb->isGood() ) {
                        $this->file->unlockAndRollback();
-                       // Something went wrong with the DB updates, so remove the target files
-                       $this->cleanupTarget( $triplets );
                        $statusDb->ok = false;
                        return $statusDb;
                }
+               wfDebugLog( 'imagemove', "Renamed {$this->file->getName()} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
+
+               // Copy the files into their new location.
+               // If a prior process fataled copying or cleaning up files we tolerate any
+               // of the existing files if they are identical to the ones being stored.
+               $statusMove = $repo->storeBatch( $triplets, FileRepo::OVERWRITE_SAME );
+               wfDebugLog( 'imagemove', "Moved files for {$this->file->getName()}: {$statusMove->successCount} successes, {$statusMove->failCount} failures" );
+               if ( !$statusMove->isGood() ) {
+                       // Delete any files copied over (while the destination is still locked)
+                       $this->cleanupTarget( $triplets );
+                       $this->file->unlockAndRollback(); // unlocks the destination
+                       wfDebugLog( 'imagemove', "Error in moving files: " . $statusMove->getWikiText() );
+                       $statusMove->ok = false;
+                       return $statusMove;
+               }
                $this->file->unlock(); // done
 
                // Everything went ok, remove the source files
@@ -2372,6 +2481,7 @@ class LocalFileMoveBatch {
 
        /**
         * Removes non-existent files from move batch.
+        * @param $triplets array
         * @return array
         */
        function removeNonexistentFiles( $triplets ) {
@@ -2403,6 +2513,7 @@ class LocalFileMoveBatch {
                // Create dest pairs from the triplets
                $pairs = array();
                foreach ( $triplets as $triplet ) {
+                       // $triplet: (old source virtual URL, dst zone, dest rel)
                        $pairs[] = array( $triplet[1], $triplet[2] );
                }