r107170 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r107169‎ | r107170 | r107171 >
Date:18:59, 23 December 2011
Author:aaron
Status:ok
Tags:
Comment:
In FileBackendBase/FileBackend:
* Changed concatenate to store to a specified temp FS file rather than a final storage destination. This makes it better fit the use case (chunked upload), so we can avoid extra copying around. Subclasses no longer have to implement this function now as well.
* Added extensionFromPath() helper function.
* Moved createInternal() up a bit and fixed @see comments pointing to the wrong functions.
In FSFileBackend:
* Use parent implementation of doConcatenateInternal().
In FileRepo/File:
* Added FileRepo::ALLOW_STALE and made thumbnail transforms use it.
Modified paths:
  • /trunk/phase3/includes/filerepo/FileRepo.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FSFileBackend.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileBackend.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileOp.php (modified) (history)
  • /trunk/phase3/includes/filerepo/file/File.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadFromChunks.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php
——@@ -349,10 +349,11 @@
350350
351351 $dest = $op['dst'];
352352 if ( $alreadyExists ) {
353 - $oldText = 'blah...blah...waahwaah';
354 - $status = $this->backend->doOperation(
355 - array( 'op' => 'create', 'content' => $oldText, 'dst' => $dest ) );
356 - $this->assertEquals( true, $status->isOK(), "Creation of file at $dest succeeded." );
 353+ $ok = file_put_contents( $dest, 'blah...blah...waahwaah' ) !== false;
 354+ $this->assertEquals( true, $ok, "Creation of file at $dest succeeded." );
 355+ } else {
 356+ $ok = file_put_contents( $dest, '' ) !== false;
 357+ $this->assertEquals( true, $ok, "Creation of 0-byte file at $dest succeeded." );
357358 }
358359
359360 // Combine them
——@@ -364,19 +365,16 @@
365366 }
366367
367368 if ( $okStatus ) {
368 - $this->assertEquals( true, $this->backend->fileExists( array( 'src' => $dest ) ),
 369+ $this->assertEquals( true, is_file( $dest ),
369370 "Dest concat file $dest exists after creation." );
370371 } else {
371 - $this->assertEquals( true, $this->backend->fileExists( array( 'src' => $dest ) ),
 372+ $this->assertEquals( true, is_file( $dest ),
372373 "Dest concat file $dest exists after failed creation." );
373374 }
374375
375 - $tmpFile = $this->backend->getLocalCopy( array( 'src' => $dest ) );
376 - $this->assertNotNull( $tmpFile, "Creation of local copy of $dest succeeded." );
 376+ $contents = file_get_contents( $dest );
 377+ $this->assertNotEquals( false, $contents, "File at $dest exists." );
377378
378 - $contents = file_get_contents( $tmpFile->getPath() );
379 - $this->assertNotEquals( false, $contents, "Local copy of $dest exists." );
380 -
381379 if ( $okStatus ) {
382380 $this->assertEquals( $expContent, $contents, "Concat file at $dest has correct contents." );
383381 } else {
——@@ -387,7 +385,8 @@
388386 function provider_testConcatenate() {
389387 $cases = array();
390388
391 - $dest = $this->singleBasePath() . '/cont1/full_file.txt';
 389+ $rand = mt_rand( 0, 2000000000 ) . time();
 390+ $dest = wfTempDir() . "/randomfile!$rand.txt";
392391 $srcs = array(
393392 $this->singleBasePath() . '/cont1/file1.txt',
394393 $this->singleBasePath() . '/cont1/file2.txt',
——@@ -430,15 +429,6 @@
431430 false, // succeeds
432431 );
433432
434 - $op['overwriteDest'] = true;
435 - $cases[] = array(
436 - $op, // operation
437 - $srcs, // sources
438 - $content, // content for each source
439 - true, // no dest already exists
440 - true, // succeeds
441 - );
442 -
443433 return $cases;
444434 }
445435
Index: trunk/phase3/includes/upload/UploadFromChunks.php
——@@ -74,7 +74,7 @@
7575
7676 $metadata = $this->stash->getMetadata( $key );
7777 $this->initializePathInfo( $name,
78 - $this->getRealPath ( $metadata['us_path'] ),
 78+ $this->getRealPath( $metadata['us_path'] ),
7979 $metadata['us_size'],
8080 false
8181 );
——@@ -86,8 +86,8 @@
8787 */
8888 public function concatenateChunks() {
8989 wfDebug( __METHOD__ . " concatenate {$this->mChunkIndex} chunks:" .
90 - $this->getOffset() . ' inx:' . $this->getChunkIndex() . "\n" );
91 -
 90+ $this->getOffset() . ' inx:' . $this->getChunkIndex() . "\n" );
 91+
9292 // Concatenate all the chunks to mVirtualTempPath
9393 $fileList = Array();
9494 // The first chunk is stored at the mVirtualTempPath path so we start on "chunk 1"
——@@ -95,13 +95,20 @@
9696 $fileList[] = $this->getVirtualChunkLocation( $i );
9797 }
9898
99 - // Concatenate into the mVirtualTempPath location;
100 - $status = $this->repo->concatenate( $fileList, $this->mVirtualTempPath, FileRepo::DELETE_SOURCE );
 99+ // Get the file extension from the last chunk
 100+ $ext = FileBackend::extensionFromPath( $this->mVirtualTempPath );
 101+ // Get a 0-byte temp file to perform the concatenation at
 102+ $tmpFile = TempFSFile::factory( 'chunkedupload_', $ext );
 103+ $tmpPath = $tmpFile
 104+ ? $tmpFile->getPath()
 105+ : false; // fail in concatenate()
 106+ // Concatenate the chunks at the temp file
 107+ $status = $this->repo->concatenate( $fileList, $tmpPath, FileRepo::DELETE_SOURCE );
101108 if( !$status->isOk() ){
102109 return $status;
103110 }
104111 // Update the mTempPath variable ( for FileUpload or normal Stash to take over )
105 - $this->mTempPath = $this->getRealPath( $this->mVirtualTempPath );
 112+ $this->mTempPath = $tmpPath; // file system path
106113 return $status;
107114 }
108115
——@@ -115,7 +122,6 @@
116123 */
117124 public function performUpload( $comment, $pageText, $watch, $user ) {
118125 $rv = parent::performUpload( $comment, $pageText, $watch, $user );
119 - $this->repo->freeTemp( $this->mVirtualTempPath );
120126 return $rv;
121127 }
122128
Index: trunk/phase3/includes/filerepo/file/File.php
——@@ -802,7 +802,7 @@
803803 // Copy any thumbnail from the FS into storage at $dstpath
804804 $status = $this->repo->store(
805805 $tmpThumbPath, 'thumb', $this->getThumbRel( $thumbName ),
806 - FileRepo::OVERWRITE | FileRepo::SKIP_LOCKING );
 806+ FileRepo::OVERWRITE | FileRepo::SKIP_LOCKING | FileRepo::ALLOW_STALE );
807807 if ( !$status->isOK() ) {
808808 return new MediaTransformError( 'thumbnail_error',
809809 $params['width'], 0, wfMsg( 'thumbnail-dest-create' ) );
Index: trunk/phase3/includes/filerepo/backend/FileOp.php
——@@ -793,19 +793,21 @@
794794 * Combines files from severals storage paths into a new file in the backend.
795795 * Parameters similar to FileBackend::concatenate(), which include:
796796 * srcs : ordered source storage paths (e.g. chunk1, chunk2, ...)
797 - * dst : destination storage path
798 - * overwriteDest : do nothing and pass if an identical file exists at destination
 797+ * dst : destination file system path to 0-byte temp file
799798 */
800799 class ConcatenateFileOp extends FileOp {
801800 protected function allowedParams() {
802 - return array( 'srcs', 'dst', 'overwriteDest' );
 801+ return array( 'srcs', 'dst' );
803802 }
804803
805804 protected function doPrecheck( array &$predicates ) {
806805 $status = Status::newGood();
807 - // Check if destination file exists
808 - $status->merge( $this->precheckDestExistence( $predicates ) );
809 - if ( !$status->isOK() ) {
 806+ // Check destination temp file
 807+ wfSuppressWarnings();
 808+ $ok = ( is_file( $this->params['dst'] ) && !filesize( $this->params['dst'] ) );
 809+ wfRestoreWarnings();
 810+ if ( !$ok ) { // not present or not empty
 811+ $status->fatal( 'backend-fail-opentemp', $this->params['dst'] );
810812 return $status;
811813 }
812814 // Check that source files exists
——@@ -815,42 +817,31 @@
816818 return $status;
817819 }
818820 }
819 - // Update file existence predicates
820 - $predicates['exists'][$this->params['dst']] = true;
821821 return $status;
822822 }
823823
824824 protected function doAttempt() {
825825 $status = Status::newGood();
826 - // Create a destination backup copy as needed
827 - if ( $this->destAlreadyExists ) {
828 - $status->merge( $this->checkAndBackupDest() );
829 - if ( !$status->isOK() ) {
830 - return $status;
831 - }
832 - }
833826 // Concatenate the file at the destination
834827 $status->merge( $this->backend->concatenateInternal( $this->params ) );
835828 return $status;
836829 }
837830
838831 protected function doRevert() {
839 - // Restore any file that was at the destination,
840 - // overwritting what was put there in attempt()
841 - return $this->restoreDest();
 832+ $status = Status::newGood();
 833+ // Clear out the temp file back to 0-bytes
 834+ wfSuppressWarnings();
 835+ $ok = file_put_contents( $this->params['dst'], '' );
 836+ wfRestoreWarnings();
 837+ if ( !$ok ) {
 838+ $status->fatal( 'backend-fail-writetemp', $this->params['dst'] );
 839+ }
 840+ return $status;
842841 }
843842
844 - protected function getSourceSha1Base36() {
845 - return null; // defer this until we finish building the new file
846 - }
847 -
848843 public function storagePathsRead() {
849844 return $this->params['srcs'];
850845 }
851 -
852 - public function storagePathsChanged() {
853 - return array( $this->params['dst'] );
854 - }
855846 }
856847
857848 /**
Index: trunk/phase3/includes/filerepo/backend/FSFileBackend.php
——@@ -225,107 +225,6 @@
226226 }
227227
228228 /**
229 - * @see FileBackend::doConcatenateInternal()
230 - */
231 - protected function doConcatenateInternal( array $params ) {
232 - $status = Status::newGood();
233 -
234 - list( $c, $dest ) = $this->resolveStoragePath( $params['dst'] );
235 - if ( $dest === null ) {
236 - $status->fatal( 'backend-fail-invalidpath', $params['dst'] );
237 - return $status;
238 - }
239 -
240 - // Check if the destination file exists and we can't handle that
241 - $destExists = file_exists( $dest );
242 - if ( $destExists && empty( $params['overwriteDest'] ) ) {
243 - $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
244 - return $status;
245 - }
246 -
247 - // Create a new temporary file...
248 - wfSuppressWarnings();
249 - $tmpPath = tempnam( wfTempDir(), 'concatenate' );
250 - wfRestoreWarnings();
251 - if ( $tmpPath === false ) {
252 - $status->fatal( 'backend-fail-createtemp' );
253 - return $status;
254 - }
255 -
256 - // Build up that file using the source chunks (in order)...
257 - wfSuppressWarnings();
258 - $tmpHandle = fopen( $tmpPath, 'a' );
259 - wfRestoreWarnings();
260 - if ( $tmpHandle === false ) {
261 - $status->fatal( 'backend-fail-opentemp', $tmpPath );
262 - return $status;
263 - }
264 - foreach ( $params['srcs'] as $virtualSource ) {
265 - list( $c, $source ) = $this->resolveStoragePath( $virtualSource );
266 - if ( $source === null ) {
267 - fclose( $tmpHandle );
268 - $status->fatal( 'backend-fail-invalidpath', $virtualSource );
269 - return $status;
270 - }
271 - // Load chunk into memory (it should be a small file)
272 - $sourceHandle = fopen( $source, 'r' );
273 - if ( $sourceHandle === false ) {
274 - fclose( $tmpHandle );
275 - $status->fatal( 'backend-fail-read', $virtualSource );
276 - return $status;
277 - }
278 - // Append chunk to file (pass chunk size to avoid magic quotes)
279 - if ( !stream_copy_to_stream( $sourceHandle, $tmpHandle ) ) {
280 - fclose( $sourceHandle );
281 - fclose( $tmpHandle );
282 - $status->fatal( 'backend-fail-writetemp', $tmpPath );
283 - return $status;
284 - }
285 - fclose( $sourceHandle );
286 - }
287 - wfSuppressWarnings();
288 - if ( !fclose( $tmpHandle ) ) {
289 - $status->fatal( 'backend-fail-closetemp', $tmpPath );
290 - return $status;
291 - }
292 - wfRestoreWarnings();
293 -
294 - // Handle overwrite behavior of file destination if applicable.
295 - // Note that we already checked if no overwrite params were set above.
296 - if ( $destExists ) {
297 - // Windows does not support moving over existing files
298 - if ( wfIsWindows() ) {
299 - wfSuppressWarnings();
300 - $ok = unlink( $dest );
301 - wfRestoreWarnings();
302 - if ( !$ok ) {
303 - $status->fatal( 'backend-fail-delete', $params['dst'] );
304 - return $status;
305 - }
306 - }
307 - } else {
308 - // Make sure destination directory exists
309 - if ( !wfMkdirParents( dirname( $dest ) ) ) {
310 - $status->fatal( 'directorycreateerror', $params['dst'] );
311 - return $status;
312 - }
313 - }
314 -
315 - // Rename the temporary file to the destination path
316 - wfSuppressWarnings();
317 - $ok = rename( $tmpPath, $dest );
318 - wfRestoreWarnings();
319 - if ( !$ok ) {
320 - $status->fatal( 'backend-fail-move', $tmpPath, $params['dst'] );
321 - return $status;
322 - }
323 -
324 - $this->chmod( $dest );
325 -
326 - return $status;
327 - }
328 -
329 - /**
330229 * @see FileBackend::doCreateInternal()
331230 */
332231 protected function doCreateInternal( array $params ) {
Index: trunk/phase3/includes/filerepo/backend/FileBackend.php
——@@ -112,8 +112,7 @@
113113 * array(
114114 * 'op' => 'concatenate',
115115 * 'srcs' => <ordered array of storage paths>,
116 - * 'dst' => <storage path>,
117 - * 'overwriteDest' => <boolean>
 116+ * 'dst' => <file system path to 0-byte temp file>
118117 * )
119118 * g) Do nothing (no-op)
120119 * array(
——@@ -477,6 +476,28 @@
478477 protected $maxCacheSize = 50; // integer; max paths with entries
479478
480479 /**
 480+ * Create a file in the backend with the given contents.
 481+ * Do not call this function from places outside FileBackend and FileOp.
 482+ * $params include:
 483+ * content : the raw file contents
 484+ * dst : destination storage path
 485+ * overwriteDest : overwrite any file that exists at the destination
 486+ *
 487+ * @param $params Array
 488+ * @return Status
 489+ */
 490+ final public function createInternal( array $params ) {
 491+ $status = $this->doCreateInternal( $params );
 492+ $this->clearCache( array( $params['dst'] ) );
 493+ return $status;
 494+ }
 495+
 496+ /**
 497+ * @see FileBackend::createInternal()
 498+ */
 499+ abstract protected function doCreateInternal( array $params );
 500+
 501+ /**
481502 * Store a file into the backend from a file on disk.
482503 * Do not call this function from places outside FileBackend and FileOp.
483504 * $params include:
——@@ -537,7 +558,7 @@
538559 }
539560
540561 /**
541 - * @see FileBackend::delete()
 562+ * @see FileBackend::deleteInternal()
542563 */
543564 abstract protected function doDeleteInternal( array $params );
544565
——@@ -559,7 +580,7 @@
560581 }
561582
562583 /**
563 - * @see FileBackend::move()
 584+ * @see FileBackend::moveInternal()
564585 */
565586 protected function doMoveInternal( array $params ) {
566587 // Copy source to dest
——@@ -591,33 +612,59 @@
592613 }
593614
594615 /**
595 - * @see FileBackend::concatenate()
 616+ * @see FileBackend::concatenateInternal()
596617 */
597 - abstract protected function doConcatenateInternal( array $params );
 618+ protected function doConcatenateInternal( array $params ) {
 619+ $status = Status::newGood();
 620+ $tmpPath = $params['dst']; // convenience
598621
599 - /**
600 - * Create a file in the backend with the given contents.
601 - * Do not call this function from places outside FileBackend and FileOp.
602 - * $params include:
603 - * content : the raw file contents
604 - * dst : destination storage path
605 - * overwriteDest : overwrite any file that exists at the destination
606 - *
607 - * @param $params Array
608 - * @return Status
609 - */
610 - final public function createInternal( array $params ) {
611 - $status = $this->doCreateInternal( $params );
612 - $this->clearCache( array( $params['dst'] ) );
 622+ // Check that the specified temp file is valid...
 623+ wfSuppressWarnings();
 624+ $ok = ( is_file( $tmpPath ) && !filesize( $tmpPath ) );
 625+ wfRestoreWarnings();
 626+ if ( !$ok ) { // not present or not empty
 627+ $status->fatal( 'backend-fail-opentemp', $tmpPath );
 628+ return $status;
 629+ }
 630+
 631+ // Build up the temp file using the source chunks (in order)...
 632+ $tmpHandle = fopen( $tmpPath, 'a' );
 633+ if ( $tmpHandle === false ) {
 634+ $status->fatal( 'backend-fail-opentemp', $tmpPath );
 635+ return $status;
 636+ }
 637+ foreach ( $params['srcs'] as $virtualSource ) {
 638+ // Get a local FS version of the chunk
 639+ $tmpFile = $this->getLocalReference( array( 'src' => $virtualSource ) );
 640+ if ( !$tmpFile ) {
 641+ $status->fatal( 'backend-fail-read', $virtualSource );
 642+ return $status;
 643+ }
 644+ // Get a handle to the local FS version
 645+ $sourceHandle = fopen( $tmpFile->getPath(), 'r' );
 646+ if ( $sourceHandle === false ) {
 647+ fclose( $tmpHandle );
 648+ $status->fatal( 'backend-fail-read', $virtualSource );
 649+ return $status;
 650+ }
 651+ // Append chunk to file (pass chunk size to avoid magic quotes)
 652+ if ( !stream_copy_to_stream( $sourceHandle, $tmpHandle ) ) {
 653+ fclose( $sourceHandle );
 654+ fclose( $tmpHandle );
 655+ $status->fatal( 'backend-fail-writetemp', $tmpPath );
 656+ return $status;
 657+ }
 658+ fclose( $sourceHandle );
 659+ }
 660+ if ( !fclose( $tmpHandle ) ) {
 661+ $status->fatal( 'backend-fail-closetemp', $tmpPath );
 662+ return $status;
 663+ }
 664+
613665 return $status;
614666 }
615667
616668 /**
617 - * @see FileBackend::create()
618 - */
619 - abstract protected function doCreateInternal( array $params );
620 -
621 - /**
622669 * @see FileBackendBase::prepare()
623670 */
624671 public function prepare( array $params ) {
——@@ -954,4 +1001,15 @@
9551002 protected function resolveContainerPath( $container, $relStoragePath ) {
9561003 return $relStoragePath;
9571004 }
 1005+
 1006+ /**
 1007+ * Get the final extension from a storage or FS path
 1008+ *
 1009+ * @param $path string
 1010+ * @return string
 1011+ */
 1012+ final public static function extensionFromPath( $path ) {
 1013+ $i = strrpos( $path, '.' );
 1014+ return strtolower( $i ? substr( $path, $i + 1 ) : '' );
 1015+ }
9581016 }
Index: trunk/phase3/includes/filerepo/FileRepo.php
——@@ -18,6 +18,7 @@
1919 const OVERWRITE = 2;
2020 const OVERWRITE_SAME = 4;
2121 const SKIP_LOCKING = 8;
 22+ const ALLOW_STALE = 16;
2223
2324 /** @var FileBackendBase */
2425 protected $backend;
——@@ -621,6 +622,7 @@
622623 * self::OVERWRITE_SAME Overwrite the file if the destination exists and has the
623624 * same contents as the source
624625 * self::SKIP_LOCKING Skip any file locking when doing the store
 626+ * self::ALLOW_STALE Don't require latest data for existence checks
625627 * @return FileRepoStatus
626628 */
627629 public function store( $srcPath, $dstZone, $dstRel, $flags = 0 ) {
——@@ -702,6 +704,9 @@
703705 if ( $flags & self::SKIP_LOCKING ) {
704706 $opts['nonLocking'] = true;
705707 }
 708+ if ( $flags & self::ALLOW_STALE ) {
 709+ $opts['allowStale'] = true;
 710+ }
706711 $status->merge( $backend->doOperations( $operations, $opts ) );
707712 // Cleanup for disk source files...
708713 foreach ( $sourceFSFilesToDelete as $file ) {
——@@ -784,7 +789,7 @@
785790 * Concatenate a list of files into a target file location.
786791 *
787792 * @param $srcPaths Array Ordered list of source virtual URLs/storage paths
788 - * @param $dstPath String Target virtual URL/storage path
 793+ * @param $dstPath String Target file system path
789794 * @param $flags Integer: bitwise combination of the following flags:
790795 * self::DELETE_SOURCE Delete the source files
791796 * @return FileRepoStatus
——@@ -806,8 +811,7 @@
807812 }
808813
809814 // Concatenate the chunks into one file
810 - $op = array( 'op' => 'concatenate',
811 - 'srcs' => $sources, 'dst' => $dest, 'overwriteDest' => true );
 815+ $op = array( 'op' => 'concatenate', 'srcs' => $sources, 'dst' => $dest );
812816 $status->merge( $this->backend->doOperation( $op ) );
813817 if ( !$status->isOK() ) {
814818 return $status;

Follow-up revisions

RevisionCommit summaryAuthorDate
r108355* Follow-up r107170: Moved FileBackend::concatenate() outside of doOperations...aaron09:25, 8 January 2012

Status & tagging log