From dcb5ec5cbf92b9a07f0776b9c194183a13400193 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 2 Mar 2016 15:42:36 -0800 Subject: [PATCH] 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 --- includes/filerepo/FileRepo.php | 12 ++++++----- includes/filerepo/LocalRepo.php | 2 +- includes/filerepo/file/File.php | 4 ++-- includes/filerepo/file/LocalFile.php | 23 +++++++++++---------- includes/upload/UploadBase.php | 30 +++++++++++++++++++++++----- includes/upload/UploadFromChunks.php | 7 +++---- 6 files changed, 51 insertions(+), 27 deletions(-) diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php index 789803f616..bd3c735f02 100644 --- 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 5e9a4a9c73..d24aa24cba 100644 --- 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 6a1bb925fd..433e39548e 100644 --- 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 c232d8266d..2a43b21baf 100644 --- 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 c1e538a235..0d6f2e2ac8 100644 --- 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 d82a9e6f9f..aa23c7db96 100644 --- 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" ); -- 2.20.1