r109583 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r109582‎ \| r109583 \| r109584 >
Date:	23:18, 19 January 2012
Author:	aaron
Status:	ok
Tags:	filebackend
Comment:	* Follow-up r109009: Check that paths are usable in FileOp::doPrecheck(). Also lock parent directories to avoid prepare()/clean() race conditions for FS backends. * Fixed bogus $params var in logException() call in SwiftFileBackend. * Added 'latest' param to FileBackendMultliWrite::consistencyCheck(). * Dummy-proof FileBackend::getFileStat() w.r.t the 'latest' param and removed related FileOp::allowStaleReads() comment. * Tweaked backend-fail-batchsize message from r109469.
Modified paths:	/trunk/phase3/includes/filerepo/backend/FSFileBackend.php (modified) (history) /trunk/phase3/includes/filerepo/backend/FileBackend.php (modified) (history) /trunk/phase3/includes/filerepo/backend/FileBackendMultiWrite.php (modified) (history) /trunk/phase3/includes/filerepo/backend/FileOp.php (modified) (history) /trunk/phase3/includes/filerepo/backend/SwiftFileBackend.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
—	—	@@ -96,6 +96,9 @@
97	97	if ( empty( $opts['nonLocking'] ) ) {
98	98	$filesLockSh = array_diff( $filesRead, $filesChanged ); // optimization
99	99	$filesLockEx = $filesChanged;
	100	+ // Get a shared lock on the parent directory of each path changed
	101	+ $filesLockSh = array_merge( $filesLockSh, array_map( 'dirname', $filesLockEx ) );
	102	+ // Try to lock those files for the scope of this function...
100	103	$scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status );
101	104	$scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );
102	105	if ( !$status->isOK() ) {
—	—	@@ -156,7 +159,7 @@
157	160
158	161	$mBackend = $this->backends[$this->masterIndex];
159	162	foreach ( array_unique( $paths ) as $path ) {
160		~~- $params = array( 'src' => $path );~~
	163	+ $params = array( 'src' => $path, 'latest' => true );
161	164	// Stat the file on the 'master' backend
162	165	$mStat = $mBackend->getFileStat( $this->substOpPaths( $params, $mBackend ) );
163	166	// Check of all clone backends agree with the master...
Index: trunk/phase3/includes/filerepo/backend/FileOp.php
—	—	@@ -55,11 +55,7 @@
56	56	}
57	57
58	58	/**
59		~~- * Allow stale data for file reads and existence checks.~~
60		- *
61		~~- * Note that we don't want to mix stale and non-stale reads~~
62		~~- * because stat calls are cached: if we read X without 'latest'~~
63		~~- * and then read it with 'latest', the data may still be stale.~~
	59	+ * Allow stale data for file reads and existence checks
64	60	*
65	61	* @return void
66	62	*/
—	—	@@ -72,11 +68,11 @@
73	69	* Callers are responsible for handling file locking.
74	70	*
75	71	* $opts is an array of options, including:
76		~~- * 'force' : Errors that would normally cause a rollback do not.~~
77		~~- * The remaining operations are still attempted if any fail.~~
78		~~- * 'allowStale' : Don't require the latest available data.~~
79		~~- * This can increase performance for non-critical writes.~~
80		~~- * This has no effect unless the 'force' flag is set.~~
	72	+ * 'force' : Errors that would normally cause a rollback do not.
	73	+ * The remaining operations are still attempted if any fail.
	74	+ * 'allowStale' : Don't require the latest available data.
	75	+ * This can increase performance for non-critical writes.
	76	+ * This has no effect unless the 'force' flag is set.
81	77	*
82	78	* @param $performOps Array List of FileOp operations
83	79	* @param $opts Array Batch operation options
—	—	@@ -155,7 +151,7 @@
156	152	/**
157	153	* Check if this operation failed precheck() or attempt()
158	154	*
159		~~- * @return type~~
	155	+ * @return bool
160	156	*/
161	157	final public function failed() {
162	158	return $this->failed;
—	—	@@ -396,20 +392,22 @@
397	393	if ( !is_file( $this->params['src'] ) ) {
398	394	$status->fatal( 'backend-fail-notexists', $this->params['src'] );
399	395	return $status;
400		~~- }~~
401	396	// Check if the source file is too big
402		~~- if ( filesize( $this->params['src'] ) > $this->backend->maxFileSizeInternal() ) {~~
403		~~- $status->fatal( 'backend-fail-store', $this->params['dst'] );~~
	397	+ } elseif ( filesize( $this->params['src'] ) > $this->backend->maxFileSizeInternal() ) {
	398	+ $status->fatal( 'backend-fail-store', $this->params['src'], $this->params['dst'] );
404	399	return $status;
	400	+ // Check if a file can be placed at the destination
	401	+ } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) {
	402	+ $status->fatal( 'backend-fail-store', $this->params['src'], $this->params['dst'] );
	403	+ return $status;
405	404	}
406	405	// Check if destination file exists
407	406	$status->merge( $this->precheckDestExistence( $predicates ) );
408		~~- if ( !$status->isOK() ) {~~
409		~~- return $status;~~
	407	+ if ( $status->isOK() ) {
	408	+ // Update file existence predicates
	409	+ $predicates['exists'][$this->params['dst']] = true;
	410	+ $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
410	411	}
411		~~- // Update file existence predicates~~
412		~~- $predicates['exists'][$this->params['dst']] = true;~~
413		~~- $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;~~
414	412	return $status; // safe to call attempt()
415	413	}
416	414
—	—	@@ -456,15 +454,18 @@
457	455	if ( strlen( $this->params['content'] ) > $this->backend->maxFileSizeInternal() ) {
458	456	$status->fatal( 'backend-fail-create', $this->params['dst'] );
459	457	return $status;
	458	+ // Check if a file can be placed at the destination
	459	+ } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) {
	460	+ $status->fatal( 'backend-fail-create', $this->params['dst'] );
	461	+ return $status;
460	462	}
461	463	// Check if destination file exists
462	464	$status->merge( $this->precheckDestExistence( $predicates ) );
463		~~- if ( !$status->isOK() ) {~~
464		~~- return $status;~~
	465	+ if ( $status->isOK() ) {
	466	+ // Update file existence predicates
	467	+ $predicates['exists'][$this->params['dst']] = true;
	468	+ $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
465	469	}
466		~~- // Update file existence predicates~~
467		~~- $predicates['exists'][$this->params['dst']] = true;~~
468		~~- $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;~~
469	470	return $status; // safe to call attempt()
470	471	}
471	472
—	—	@@ -505,15 +506,18 @@
506	507	if ( !$this->fileExists( $this->params['src'], $predicates ) ) {
507	508	$status->fatal( 'backend-fail-notexists', $this->params['src'] );
508	509	return $status;
	510	+ // Check if a file can be placed at the destination
	511	+ } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) {
	512	+ $status->fatal( 'backend-fail-copy', $this->params['src'], $this->params['dst'] );
	513	+ return $status;
509	514	}
510	515	// Check if destination file exists
511	516	$status->merge( $this->precheckDestExistence( $predicates ) );
512		~~- if ( !$status->isOK() ) {~~
513		~~- return $status;~~
	517	+ if ( $status->isOK() ) {
	518	+ // Update file existence predicates
	519	+ $predicates['exists'][$this->params['dst']] = true;
	520	+ $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
514	521	}
515		~~- // Update file existence predicates~~
516		~~- $predicates['exists'][$this->params['dst']] = true;~~
517		~~- $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;~~
518	522	return $status; // safe to call attempt()
519	523	}
520	524
—	—	@@ -557,17 +561,20 @@
558	562	if ( !$this->fileExists( $this->params['src'], $predicates ) ) {
559	563	$status->fatal( 'backend-fail-notexists', $this->params['src'] );
560	564	return $status;
	565	+ // Check if a file can be placed at the destination
	566	+ } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) {
	567	+ $status->fatal( 'backend-fail-move', $this->params['src'], $this->params['dst'] );
	568	+ return $status;
561	569	}
562	570	// Check if destination file exists
563	571	$status->merge( $this->precheckDestExistence( $predicates ) );
564		~~- if ( !$status->isOK() ) {~~
565		~~- return $status;~~
	572	+ if ( $status->isOK() ) {
	573	+ // Update file existence predicates
	574	+ $predicates['exists'][$this->params['src']] = false;
	575	+ $predicates['sha1'][$this->params['src']] = false;
	576	+ $predicates['exists'][$this->params['dst']] = true;
	577	+ $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
566	578	}
567		~~- // Update file existence predicates~~
568		~~- $predicates['exists'][$this->params['src']] = false;~~
569		~~- $predicates['sha1'][$this->params['src']] = false;~~
570		~~- $predicates['exists'][$this->params['dst']] = true;~~
571		~~- $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;~~
572	579	return $status; // safe to call attempt()
573	580	}
574	581
—	—	@@ -582,9 +589,6 @@
583	590	// Just delete source as the destination needs no changes
584	591	$params = array( 'src' => $this->params['src'] );
585	592	$status->merge( $this->backend->deleteInternal( $params ) );
586		~~- if ( !$status->isOK() ) {~~
587		~~- return $status;~~
588		~~- }~~
589	593	}
590	594	}
591	595	return $status;
—	—	@@ -633,9 +637,6 @@
634	638	if ( $this->needsDelete ) {
635	639	// Delete the source file
636	640	$status->merge( $this->backend->deleteInternal( $this->params ) );
637		~~- if ( !$status->isOK() ) {~~
638		~~- return $status;~~
639		~~- }~~
640	641	}
641	642	return $status;
642	643	}
Index: trunk/phase3/includes/filerepo/backend/FSFileBackend.php
—	—	@@ -102,6 +102,27 @@
103	103	}
104	104
105	105	/**
	106	+ * @see FileBackend::isPathUsableInternal()
	107	+ */
	108	+ public function isPathUsableInternal( $storagePath ) {
	109	+ $fsPath = $this->resolveToFSPath( $storagePath );
	110	+ if ( $fsPath === null ) {
	111	+ return false; // invalid
	112	+ }
	113	+ $parentDir = dirname( $fsPath );
	114	+
	115	+ wfSuppressWarnings();
	116	+ if ( file_exists( $fsPath ) ) {
	117	+ $ok = is_file( $fsPath ) && is_writable( $fsPath );
	118	+ } else {
	119	+ $ok = is_dir( $parentDir ) && is_writable( $parentDir );
	120	+ }
	121	+ wfRestoreWarnings();
	122	+
	123	+ return $ok;
	124	+ }
	125	+
	126	+ /**
106	127	* @see FileBackend::doStoreInternal()
107	128	*/
108	129	protected function doStoreInternal( array $params ) {
Index: trunk/phase3/includes/filerepo/backend/SwiftFileBackend.php
—	—	@@ -73,6 +73,27 @@
74	74	}
75	75
76	76	/**
	77	+ * @see FileBackend::isPathUsableInternal()
	78	+ */
	79	+ public function isPathUsableInternal( $storagePath ) {
	80	+ list( $container, $rel ) = $this->resolveStoragePathReal( $storagePath );
	81	+ if ( $rel === null ) {
	82	+ return false; // invalid
	83	+ }
	84	+
	85	+ try {
	86	+ $this->getContainer( $container );
	87	+ return true; // container exists
	88	+ } catch ( NoSuchContainerException $e ) {
	89	+ } catch ( InvalidResponseException $e ) {
	90	+ } catch ( Exception $e ) { // some other exception?
	91	+ $this->logException( $e, __METHOD__, array( 'path' => $storagePath ) );
	92	+ }
	93	+
	94	+ return false;
	95	+ }
	96	+
	97	+ /**
77	98	* @see FileBackend::doCopyInternal()
78	99	*/
79	100	protected function doCreateInternal( array $params ) {
—	—	@@ -492,7 +513,7 @@
493	514	} catch ( NoSuchObjectException $e ) {
494	515	} catch ( InvalidResponseException $e ) {
495	516	} catch ( Exception $e ) { // some other exception?
496		~~- $this->logException( $e, __METHOD__, $params );~~
	517	+ $this->logException( $e, __METHOD__, array( 'cont' => $fullCont, 'dir' => $dir ) );
497	518	}
498	519
499	520	return $files;
Index: trunk/phase3/includes/filerepo/backend/FileBackend.php
—	—	@@ -419,6 +419,7 @@
420	420	* Otherwise, the result is an associative array that includes:
421	421	* mtime : the last-modified timestamp (TS_MW)
422	422	* size : the file size (bytes)
	423	+ * Additional values may be included for internal use only.
423	424	*
424	425	* $params include:
425	426	* src : source storage path
—	—	@@ -577,11 +578,11 @@
578	579	* This class defines the methods as abstract that subclasses must implement.
579	580	* Callers outside of FileBackend and its helper classes, such as FileOp,
580	581	* should only call functions that are present in FileBackendBase.
581		- *
	582	+ *
582	583	* The FileBackendBase operations are implemented using primitive functions
583	584	* such as storeInternal(), copyInternal(), deleteInternal() and the like.
584	585	* This class is also responsible for path resolution and sanitization.
585		- *
	586	+ *
586	587	* @ingroup FileBackend
587	588	* @since 1.19
588	589	*/
—	—	@@ -606,6 +607,16 @@
607	608	}
608	609
609	610	/**
	611	+ * Check if a file can be created at a given storage path.
	612	+ * FS backends should check if the parent directory exists and the file is writable.
	613	+ * Backends using key/value stores should check if the container exists.
	614	+ *
	615	+ * @param $storagePath string
	616	+ * @return bool
	617	+ */
	618	+ abstract public function isPathUsableInternal( $storagePath );
	619	+
	620	+ /**
610	621	* Create a file in the backend with the given contents.
611	622	* Do not call this function from places outside FileBackend and FileOp.
612	623	*
—	—	@@ -878,6 +889,12 @@
879	890	$status->fatal( 'backend-fail-invalidpath', $params['dir'] );
880	891	return $status; // invalid storage path
881	892	}
	893	+ // Attempt to lock this directory...
	894	+ $filesLockEx = array( $params['dir'] );
	895	+ $scopedLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );
	896	+ if ( !$status->isOK() ) {
	897	+ return $status; // abort
	898	+ }
882	899	if ( $shard !== null ) { // confined to a single container/shard
883	900	$status->merge( $this->doCleanInternal( $fullCont, $dir, $params ) );
884	901	} else { // directory is on several shards
—	—	@@ -937,13 +954,19 @@
938	955	*/
939	956	final public function getFileStat( array $params ) {
940	957	$path = $params['src'];
	958	+ $latest = !empty( $params['latest'] );
941	959	if ( isset( $this->cache[$path]['stat'] ) ) {
942		~~- return $this->cache[$path]['stat'];~~
	960	+ // If we want the latest data, check that this cached
	961	+ // value was in fact fetched with the latest available data.
	962	+ if ( !$latest \|\| $this->cache[$path]['stat']['latest'] ) {
	963	+ return $this->cache[$path]['stat'];
	964	+ }
943	965	}
944	966	$stat = $this->doGetFileStat( $params );
945	967	if ( is_array( $stat ) ) { // don't cache negatives
946	968	$this->trimCache(); // limit memory
947	969	$this->cache[$path]['stat'] = $stat;
	970	+ $this->cache[$path]['stat']['latest'] = $latest;
948	971	}
949	972	return $stat;
950	973	}
—	—	@@ -1161,6 +1184,8 @@
1162	1185	}
1163	1186	// Optimization: if doing an EX lock anyway, don't also set an SH one
1164	1187	$filesLockSh = array_diff( $filesLockSh, $filesLockEx );
	1188	+ // Get a shared lock on the parent directory of each path changed
	1189	+ $filesLockSh = array_merge( $filesLockSh, array_map( 'dirname', $filesLockEx ) );
1165	1190	// Try to lock those files for the scope of this function...
1166	1191	$scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status );
1167	1192	$scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );

Follow-up revisions

Revision	Commit summary	Author	Date
r109591	Message change I forgot to commit in r109583	aaron	00:12, 20 January 2012

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r109009	In FileOp/FileBackend:...	aaron	22:45, 15 January 2012
r109469	In FileBackend/FileOp:...	aaron	02:07, 19 January 2012

Status & tagging log

03:52, 1 February 2012 Tim Starling (talk | contribs) changed the status of r109583 [removed: new added: ok]
12:27, 22 January 2012 Siebrand (talk | contribs) changed the tags for r109583 [added: filebackend]