Make UploadBase use TempFSFile to wrap the temporary file

* The cleanupTempFile() method now only marks it as ready
  for unlinking when unreferenced.
* Pass TempFSFile down the FileRepo call chain so that it
  can build off a5d903860a and allow more fast async writes
  for secondary backends. Previously, the 'store' operation
  used on upload forced sync file change replication writes.
* Also fix bogus method call in LocalFile::publishTo().

Bug: T91869
Change-Id: I06caa6e5d8bdec9a7cae2b68cb02dd2e64b9ac74

diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php
index 789803f..bd3c735 100644 (file)
--- a/includes/filerepo/FileRepo.php
+++ b/includes/filerepo/FileRepo.php
@@ -1158,7 +1158,7 @@ class FileRepo {
         * Options to $options include:
         *   - headers : name/value map of HTTP headers to use in response to GET/HEAD requests
         *
-        * @param string $srcPath The source file system path, storage path, or URL
+        * @param string|FSFile $src The source file system path, storage path, or URL
         * @param string $dstRel The destination relative path
         * @param string $archiveRel The relative path where the existing file is to
         *   be archived, if there is one. Relative to the public zone root.
@@ -1168,12 +1168,12 @@ class FileRepo {
         * @return FileRepoStatus
         */
        public function publish(
-               $srcPath, $dstRel, $archiveRel, $flags = 0, array $options = []
+               $src, $dstRel, $archiveRel, $flags = 0, array $options = []
        ) {
                $this->assertWritableRepo(); // fail out if read-only
 
                $status = $this->publishBatch(
-                       [ [ $srcPath, $dstRel, $archiveRel, $options ] ], $flags );
+                       [ [ $src, $dstRel, $archiveRel, $options ] ], $flags );
                if ( $status->successCount == 0 ) {
                        $status->ok = false;
                }
@@ -1212,7 +1212,9 @@ class FileRepo {
                $sourceFSFilesToDelete = []; // cleanup for disk source files
                // Validate each triplet and get the store operation...
                foreach ( $ntuples as $ntuple ) {
-                       list( $srcPath, $dstRel, $archiveRel ) = $ntuple;
+                       list( $src, $dstRel, $archiveRel ) = $ntuple;
+                       $srcPath = ( $src instanceof FSFile ) ? $src->getPath() : $src;
+
                        $options = isset( $ntuple[3] ) ? $ntuple[3] : [];
                        // Resolve source to a storage path if virtual
                        $srcPath = $this->resolveToStoragePath( $srcPath );
@@ -1275,7 +1277,7 @@ class FileRepo {
                        } else { // FS source path
                                $operations[] = [
                                        'op' => 'store',
-                                       'src' => $srcPath,
+                                       'src' => $src, // prefer FSFile objects
                                        'dst' => $dstPath,
                                        'overwrite' => true, // replace current
                                        'headers' => $headers

diff --git a/includes/filerepo/LocalRepo.php b/includes/filerepo/LocalRepo.php
index 5e9a4a9..d24aa24 100644 (file)
--- a/includes/filerepo/LocalRepo.php
+++ b/includes/filerepo/LocalRepo.php
@@ -532,7 +532,7 @@ class LocalRepo extends FileRepo {
        }
 
        public function publish(
-               $srcPath,
+               $src,
                $dstRel,
                $archiveRel,
                $flags = 0,

diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php
index 6a1bb92..433e395 100644 (file)
--- a/includes/filerepo/file/File.php
+++ b/includes/filerepo/file/File.php
@@ -1802,7 +1802,7 @@ abstract class File implements IDBAccessObject {
         * Options to $options include:
         *   - headers : name/value map of HTTP headers to use in response to GET/HEAD requests
         *
-        * @param string $srcPath Local filesystem path to the source image
+        * @param string|FSFile $src Local filesystem path to the source image
         * @param int $flags A bitwise combination of:
         *   File::DELETE_SOURCE    Delete the source file, i.e. move rather than copy
         * @param array $options Optional additional parameters
@@ -1812,7 +1812,7 @@ abstract class File implements IDBAccessObject {
         * STUB
         * Overridden by LocalFile
         */
-       function publish( $srcPath, $flags = 0, array $options = [] ) {
+       function publish( $src, $flags = 0, array $options = [] ) {
                $this->readOnlyError();
        }
 

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index c232d82..2a43b21 100644 (file)
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -1108,7 +1108,7 @@ class LocalFile extends File {
 
        /**
         * Upload a file and record it in the DB
-        * @param string $srcPath Source storage path, virtual URL, or filesystem path
+        * @param string|FSFile $src Source storage path, virtual URL, or filesystem path
         * @param string $comment Upload description
         * @param string $pageText Text to use for the new description page,
         *   if a new description page is created
@@ -1124,7 +1124,7 @@ class LocalFile extends File {
         * @return FileRepoStatus On success, the value member contains the
         *     archive name, or an empty string if it was a new file.
         */
-       function upload( $srcPath, $comment, $pageText, $flags = 0, $props = false,
+       function upload( $src, $comment, $pageText, $flags = 0, $props = false,
                $timestamp = false, $user = null, $tags = []
        ) {
                global $wgContLang;
@@ -1133,6 +1133,7 @@ class LocalFile extends File {
                        return $this->readOnlyFatalStatus();
                }
 
+               $srcPath = ( $src instanceof FSFile ) ? $src->getPath() : $src;
                if ( !$props ) {
                        if ( $this->repo->isVirtualUrl( $srcPath )
                                || FileBackend::isStoragePath( $srcPath )
@@ -1158,7 +1159,7 @@ class LocalFile extends File {
                // non-nicely (dangling multi-byte chars, non-truncated version in cache).
                $comment = $wgContLang->truncate( $comment, 255 );
                $this->lock(); // begin
-               $status = $this->publish( $srcPath, $flags, $options );
+               $status = $this->publish( $src, $flags, $options );
 
                if ( $status->successCount >= 2 ) {
                        // There will be a copy+(one of move,copy,store).
@@ -1527,15 +1528,15 @@ class LocalFile extends File {
         * The archive name should be passed through to recordUpload for database
         * registration.
         *
-        * @param string $srcPath Local filesystem path or virtual URL to the source image
+        * @param string|FSFile $src Local filesystem path or virtual URL to the source image
         * @param int $flags A bitwise combination of:
         *     File::DELETE_SOURCE    Delete the source file, i.e. move rather than copy
         * @param array $options Optional additional parameters
         * @return FileRepoStatus On success, the value member contains the
         *     archive name, or an empty string if it was a new file.
         */
-       function publish( $srcPath, $flags = 0, array $options = [] ) {
-               return $this->publishTo( $srcPath, $this->getRel(), $flags, $options );
+       function publish( $src, $flags = 0, array $options = [] ) {
+               return $this->publishTo( $src, $this->getRel(), $flags, $options );
        }
 
        /**
@@ -1545,7 +1546,7 @@ class LocalFile extends File {
         * The archive name should be passed through to recordUpload for database
         * registration.
         *
-        * @param string $srcPath Local filesystem path or virtual URL to the source image
+        * @param string|FSFile $src Local filesystem path or virtual URL to the source image
         * @param string $dstRel Target relative path
         * @param int $flags A bitwise combination of:
         *     File::DELETE_SOURCE    Delete the source file, i.e. move rather than copy
@@ -1553,7 +1554,9 @@ class LocalFile extends File {
         * @return FileRepoStatus On success, the value member contains the
         *     archive name, or an empty string if it was a new file.
         */
-       function publishTo( $srcPath, $dstRel, $flags = 0, array $options = [] ) {
+       function publishTo( $src, $dstRel, $flags = 0, array $options = [] ) {
+               $srcPath = ( $src instanceof FSFile ) ? $src->getPath() : $src;
+
                $repo = $this->getRepo();
                if ( $repo->getReadOnlyReason() !== false ) {
                        return $this->readOnlyFatalStatus();
@@ -1567,9 +1570,9 @@ class LocalFile extends File {
                if ( $repo->hasSha1Storage() ) {
                        $sha1 = $repo->isVirtualUrl( $srcPath )
                                ? $repo->getFileSha1( $srcPath )
-                               : File::sha1Base36( $srcPath );
+                               : FSFile::getSha1Base36FromPath( $srcPath );
                        $dst = $repo->getBackend()->getPathForSHA1( $sha1 );
-                       $status = $repo->quickImport( $srcPath, $dst );
+                       $status = $repo->quickImport( $src, $dst );
                        if ( $flags & File::DELETE_SOURCE ) {
                                unlink( $srcPath );
                        }

diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php
index c1e538a..0d6f2e2 100644 (file)
--- a/includes/upload/UploadBase.php
+++ b/includes/upload/UploadBase.php
@@ -36,7 +36,11 @@
  * @author Michael Dale
  */
 abstract class UploadBase {
+       /** @var string Local file system path to the file to upload (or a local copy) */
        protected $mTempPath;
+       /** @var TempFSFile|null Wrapper to handle deleting the temp file */
+       protected $tempFileObj;
+
        protected $mDesiredDestName, $mDestName, $mRemoveTempFile, $mSourceType;
        protected $mTitle = false, $mTitleError = 0;
        protected $mFilteredName, $mFinalExtension;
@@ -219,8 +223,8 @@ abstract class UploadBase {
                if ( FileBackend::isStoragePath( $tempPath ) ) {
                        throw new MWException( __METHOD__ . " given storage path `$tempPath`." );
                }
-               $this->mTempPath = $tempPath;
-               $this->mFileSize = $fileSize;
+
+               $this->setTempFile( $tempPath, $fileSize );
                $this->mRemoveTempFile = $removeTempFile;
        }
 
@@ -231,6 +235,21 @@ abstract class UploadBase {
         */
        abstract public function initializeFromRequest( &$request );
 
+       /**
+        * @param string $tempPath File system path to temporary file containing the upload
+        * @param integer $fileSize
+        */
+       protected function setTempFile( $tempPath, $fileSize = null ) {
+               $this->mTempPath = $tempPath;
+               if ( strlen( $this->mTempPath ) && file_exists( $this->mTempPath ) ) {
+                       $this->tempFileObj = new TempFSFile( $this->mTempPath );
+                       $this->mFileSize = $fileSize ?: filesize( $this->mTempPath );
+               } else {
+                       $this->tempFileObj = null;
+                       $this->mFileSize = null;
+               }
+       }
+
        /**
         * Fetch the file. Usually a no-op
         * @return Status
@@ -952,9 +971,10 @@ abstract class UploadBase {
         * on exit to clean up.
         */
        public function cleanupTempFile() {
-               if ( $this->mRemoveTempFile && $this->mTempPath && file_exists( $this->mTempPath ) ) {
-                       wfDebug( __METHOD__ . ": Removing temporary file {$this->mTempPath}\n" );
-                       unlink( $this->mTempPath );
+               if ( $this->mRemoveTempFile && $this->tempFileObj ) {
+                       // Delete when all relevant TempFSFile handles go out of scope
+                       wfDebug( __METHOD__ . ": Marked temporary file '{$this->mTempPath}' for removal\n" );
+                       $this->tempFileObj->autocollect();
                }
        }
 

diff --git a/includes/upload/UploadFromChunks.php b/includes/upload/UploadFromChunks.php
index d82a9e6..aa23c7d 100644 (file)
--- a/includes/upload/UploadFromChunks.php
+++ b/includes/upload/UploadFromChunks.php
@@ -147,10 +147,9 @@ class UploadFromChunks extends UploadFromFile {
                }
                wfDebugLog( 'fileconcatenate', "Combined $i chunks in $tAmount seconds." );
 
-               // File system path
-               $this->mTempPath = $tmpPath;
-               // Since this was set for the last chunk previously
-               $this->mFileSize = filesize( $this->mTempPath );
+               // File system path of the actual full temp file
+               $this->setTempFile( $tmpPath );
+
                $ret = $this->verifyUpload();
                if ( $ret['status'] !== UploadBase::OK ) {
                        wfDebugLog( 'fileconcatenate', "Verification failed for chunked upload" );