r105846 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r105845‎ \| r105846 \| r105847 >
Date:	00:50, 12 December 2011
Author:	aaron
Status:	deferred
Tags:
Comment:	* Removed 'cache' as an option to DBLockManager (always used now); makes construction simpler * Made TempFSFile::factory() perform more consistently and avoided race condition with rename() (on UNIX) * Use file basename for FSFileBackend local copy backup prefixes (might help for recovery) * Remove now-unneeded gmdate() call in File::maybeDoTransform which was causing regenerations for no reason * Avoid creating temp files in File::maybeDoTransform when we are just going to call getTransform() and not actually write anything * Updated EXIF tests to use FORCE_RENDER since they require an output FS file
Modified paths:	/branches/FileBackend/phase3/includes/filerepo/backend/FSFileBackend.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/backend/LockManager.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/file/File.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/file/TempFSFile.php (modified) (history) /branches/FileBackend/phase3/tests/phpunit/includes/media/ExifRotationTest.php (modified) (history)

Diff [purge]

Index: branches/FileBackend/phase3/tests/phpunit/includes/media/ExifRotationTest.php
—	—	@@ -16,9 +16,9 @@
17	17	'containerPaths' => array( 'images-thumb' => $tmpDir, 'data' => $filePath )
18	18	) );
19	19	$this->repo = new FSRepo( array(
20		~~- 'name' => 'temp',~~
21		~~- 'url' => 'http://localhost/thumbtest',~~
22		~~- 'backend' => $this->backend~~
	20	+ 'name' => 'temp',
	21	+ 'url' => 'http://localhost/thumbtest',
	22	+ 'backend' => $this->backend
23	23	) );
24	24	if ( !wfDl( 'exif' ) ) {
25	25	$this->markTestSkipped( "This test needs the exif extension." );
—	—	@@ -73,7 +73,7 @@
74	74	}
75	75
76	76	$file = $this->dataFile( $name, $type );
77		~~- $thumb = $file->transform( $params, File::RENDER_NOW );~~
	77	+ $thumb = $file->transform( $params, File::RENDER_NOW \| File::RENDER_FORCE );
78	78
79	79	$this->assertEquals( $out[0], $thumb->getWidth(), "$name: thumb reported width check for $size" );
80	80	$this->assertEquals( $out[1], $thumb->getHeight(), "$name: thumb reported height check for $size" );
—	—	@@ -166,7 +166,7 @@
167	167	}
168	168
169	169	$file = $this->dataFile( $name, $type );
170		~~- $thumb = $file->transform( $params, File::RENDER_NOW );~~
	170	+ $thumb = $file->transform( $params, File::RENDER_NOW \| File::RENDER_FORCE );
171	171
172	172	$this->assertEquals( $out[0], $thumb->getWidth(), "$name: thumb reported width check for $size" );
173	173	$this->assertEquals( $out[1], $thumb->getHeight(), "$name: thumb reported height check for $size" );
Index: branches/FileBackend/phase3/includes/filerepo/file/TempFSFile.php
—	—	@@ -26,21 +26,23 @@
27	27	* @return TempFSFile\|null
28	28	*/
29	29	public static function factory( $prefix, $extension = '' ) {
30		~~- $tmpDest = tempnam( wfTempDir(), $prefix );~~
31		~~- if ( $tmpDest === false ) {~~
32		~~- return null;~~
33		~~- }~~
34		~~- if ( $extension != '' ) {~~
35		~~- $path = "{$tmpDest}.{$extension}";~~
36		~~- if ( !rename( $tmpDest, $path ) ) {~~
37		~~- return null;~~
	30	+ $base = wfTempDir() . '/' . $prefix . dechex( mt_rand( 0, 99999999 ) );
	31	+ $ext = ( $extension != '' ) ? ".{$extension}" : "";
	32	+ for ( $attempt = 1; true; $attempt++ ) {
	33	+ $path = "{$base}-{$attempt}{$ext}";
	34	+ wfSuppressWarnings();
	35	+ $newFileHandle = fopen( $path, 'x' );
	36	+ wfRestoreWarnings();
	37	+ if ( $newFileHandle ) {
	38	+ fclose( $newFileHandle );
	39	+ break; // got it
38	40	}
39		~~- } else {~~
40		~~- $path = $tmpDest;~~
	41	+ if ( $attempt >= 15 ) {
	42	+ return null; // give up
	43	+ }
41	44	}
42	45	$tmpFile = new self( $path );
43		~~- self::$instances[] = $tmpFile;~~
44		-
	46	+ self::$instances[] = $tmpFile; // defer purge till shutdown
45	47	return $tmpFile;
46	48	}
47	49
Index: branches/FileBackend/phase3/includes/filerepo/file/File.php
—	—	@@ -757,30 +757,31 @@
758	758	global $wgIgnoreImageErrors, $wgThumbnailEpoch;
759	759
760	760	$thumbPath = $this->getThumbPath( $thumbName ); // final thumb path
761		~~- // Create a temp FS file with the same extension~~
762		~~- $tmpFile = TempFSFile::factory( 'transform', $this->getExtension() );~~
763		~~- if ( !$tmpFile ) {~~
764		~~- return new MediaTransformError( 'thumbnail_error',~~
765		~~- $params['width'], 0, wfMsg( 'thumbnail-temp-create' ) );~~
766		~~- }~~
767		~~- $tmpThumbPath = $tmpFile->getPath(); // path of 0-byte temp file~~
768	761
769	762	if ( $this->repo && $this->repo->canTransformVia404() && !( $flags & self::RENDER_NOW ) ) {
770	763	wfDebug( __METHOD__ . " transformation deferred." );
771		~~- return $this->handler->getTransform( $this, $tmpThumbPath, $thumbUrl, $params );~~
	764	+ return $this->handler->getTransform( $this, false, $thumbUrl, $params );
772	765	}
773	766
774	767	wfDebug( __METHOD__.": Doing stat for $thumbPath\n" );
775	768	$this->migrateThumbFile( $thumbName );
776	769	if ( $this->repo->fileExists( $thumbPath ) && !( $flags & self::RENDER_FORCE ) ) {
777		~~- $ts = $this->repo->getFileTimestamp( $thumbPath );~~
778		~~- if ( $ts !== false && gmdate( 'YmdHis', $ts ) >= $wgThumbnailEpoch ) {~~
779		~~- return $this->handler->getTransform( $this, $tmpThumbPath, $thumbUrl, $params );~~
	770	+ $timestamp = $this->repo->getFileTimestamp( $thumbPath );
	771	+ if ( $timestamp !== false && $timestamp >= $wgThumbnailEpoch ) {
	772	+ return $this->handler->getTransform( $this, false, $thumbUrl, $params );
780	773	}
781	774	} elseif ( $flags & self::RENDER_FORCE ) {
782	775	wfDebug( __METHOD__ . " forcing rendering per flag File::RENDER_FORCE\n" );
783	776	}
784	777
	778	+ // Create a temp FS file with the same extension
	779	+ $tmpFile = TempFSFile::factory( 'transform_', $this->getExtension() );
	780	+ if ( !$tmpFile ) {
	781	+ return new MediaTransformError( 'thumbnail_error',
	782	+ $params['width'], 0, wfMsg( 'thumbnail-temp-create' ) );
	783	+ }
	784	+ $tmpThumbPath = $tmpFile->getPath(); // path of 0-byte temp file
	785	+
785	786	// Actually render the thumbnail
786	787	$thumb = $this->handler->doTransform( $this, $tmpThumbPath, $thumbUrl, $params );
787	788
Index: branches/FileBackend/phase3/includes/filerepo/backend/FSFileBackend.php
—	—	@@ -484,7 +484,7 @@
485	485	$i = strrpos( $source, '.' );
486	486	$ext = strtolower( $i ? substr( $source, $i + 1 ) : '' );
487	487	// Create a new temporary file...
488		~~- $tmpFile = TempFSFile::factory( 'localcopy_', $ext );~~
	488	+ $tmpFile = TempFSFile::factory( wfBaseName( $source ) . '_', $ext );
489	489	if ( !$tmpFile ) {
490	490	return null;
491	491	}
Index: branches/FileBackend/phase3/includes/filerepo/backend/LockManager.php
—	—	@@ -318,8 +318,6 @@
319	319	* 'safeDelay' : Seconds to mistrust a DB after restart/query loss. [optional]
320	320	* This should reflect the highest max_execution_time that PHP
321	321	* scripts might use on a wiki. Locks are lost on DB server restart.
322		~~- * 'cache' : $wgMemc (if set to a global memcached instance). [optional]~~
323		~~- * This tracks peer DBs that couldn't be queried recently.~~
324	322	*
325	323	* @param Array $config
326	324	*/
—	—	@@ -341,11 +339,7 @@
342	340	? $config['safeDelay']
343	341	: max( $this->cliTimeout, $this->webTimeout ); // cover worst case
344	342
345		~~- if ( isset( $config['cache'] ) && $config['cache'] instanceof BagOStuff ) {~~
346		~~- $this->statusCache = $config['cache'];~~
347		~~- } else {~~
348		~~- $this->statusCache = null;~~
349		~~- }~~
	343	+ $this->statusCache = wfGetMainCache(); // tracks peers that couldn't be queried recently
350	344	}
351	345
352	346	protected function doLock( array $keys, $type ) {
—	—	@@ -687,13 +681,13 @@
688	682	}
689	683	# Block new writers...
690	684	$db->insert( 'file_locks_shared', $data, __METHOD__ );
691		~~- # Wait on any existing writers...~~
692		~~- $db->query( "SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED" );~~
	685	+ # Bail if there are any existing writers...
	686	+ $db->query( "SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;" );
693	687	$ok = !$db->selectField( 'file_locks_exclusive', '1',
694	688	array( 'fle_key' => $keys ),
695	689	__METHOD__
696	690	);
697		~~- $db->query( "SET SESSION TRANSACTION ISOLATION LEVEL {$this->trxIso[$lockDb]}" );~~
	691	+ $db->query( "SET SESSION TRANSACTION ISOLATION LEVEL {$this->trxIso[$lockDb]};" );
698	692	} elseif ( $type == self::LOCK_EX ) { // writer locks
699	693	$db = $this->getConnection( $lockDb );
700	694	$data = array();
—	—	@@ -702,13 +696,13 @@
703	697	}
704	698	# Block new readers/writers and wait on any existing writers
705	699	$db->insert( 'file_locks_exclusive', $data, __METHOD__ );
706		~~- # Wait on any existing readers...~~
707		~~- $db->query( "SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED" );~~
	700	+ # Bail if there are any existing readers...
	701	+ $db->query( "SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;" );
708	702	$ok = !$db->selectField( 'file_locks_shared', '1',
709	703	array( 'fls_key' => $keys ),
710	704	__METHOD__
711	705	);
712		~~- $db->query( "SET SESSION TRANSACTION ISOLATION LEVEL {$this->trxIso[$lockDb]}" );~~
	706	+ $db->query( "SET SESSION TRANSACTION ISOLATION LEVEL {$this->trxIso[$lockDb]};" );
713	707	}
714	708	return $ok;
715	709	}

Status & tagging log

22:45, 21 December 2011 Tim Starling (talk | contribs) changed the status of r105846 [removed: new added: deferred]