r103877 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r103876‎ \| r103877 \| r103878 >
Date:	00:32, 22 November 2011
Author:	aaron
Status:	deferred
Tags:
Comment:	FileLockManager: * Simplified scheme for resource names used by locks * Added LOCK_SH and LOCK_EX options * Fixed FSFileLockManager::doLock() rollback logic * Use Status objects correctly
Modified paths:	/branches/FileBackend/phase3/includes/filerepo/backend/FSFileBackend.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/backend/FileLockManager.php (modified) (history)

Diff [purge]

Index: branches/FileBackend/phase3/includes/filerepo/backend/FSFileBackend.php
—	—	@@ -34,7 +34,7 @@
35	35
36	36	list( $c, $dest ) = $this->resolveVirtualPath( $params['dest'] );
37	37	if ( $dest === null ) {
38		~~- $status->fatal( "Invalid storage path {$params['dest']}." );~~
	38	+ $status->fatal( 'backend-fail-invalidpath', $params['dest'] );
39	39	return $status;
40	40	}
41	41
—	—	@@ -42,16 +42,16 @@
43	43	if ( isset( $params['overwriteDest'] ) ) {
44	44	$ok = unlink( $dest );
45	45	if ( !$ok ) {
46		~~- $status->fatal( "Could not delete destination file." );~~
	46	+ $status->fatal( 'backend-fail-delete', $param['dest'] );
47	47	return $status;
48	48	}
49	49	} elseif ( isset( $params['overwriteSame'] ) ) {
50	50	if ( !$this->filesAreSame( $params['source'], $dest ) ) {
51		~~- $status->fatal( "Non-identical destination file already exists." );~~
	51	+ $status->fatal( 'backend-fail-notsame', $params['source'], $params['dest'] );
52	52	}
53	53	return $status; // do nothing; either OK or bad status
54	54	} else {
55		~~- $status->fatal( "Destination file already exists." );~~
	55	+ $status->fatal( 'backend-fail-alreadyexists', $params['dest'] );
56	56	return $status;
57	57	}
58	58	} else {
—	—	@@ -62,7 +62,7 @@
63	63	$ok = copy( $params['source'], $dest );
64	64	wfRestoreWarnings();
65	65	if ( !$ok ) {
66		~~- $status->fatal( "Could not copy file to destination." );~~
	66	+ $status->fatal( 'backend-fail-copy', $params['source'], $params['dest'] );
67	67	return $status;
68	68	}
69	69
—	—	@@ -80,12 +80,12 @@
81	81
82	82	list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );
83	83	if ( $source === null ) {
84		~~- $status->fatal( "Invalid storage path {$params['source']}." );~~
	84	+ $status->fatal( 'backend-fail-invalidpath', $params['source'] );
85	85	return $status;
86	86	}
87	87	list( $c, $dest ) = $this->resolveVirtualPath( $params['dest'] );
88	88	if ( $dest === null ) {
89		~~- $status->fatal( "Invalid storage path {$params['dest']}." );~~
	89	+ $status->fatal( 'backend-fail-invalidpath', $params['dest'] );
90	90	return $status;
91	91	}
92	92
—	—	@@ -93,16 +93,16 @@
94	94	if ( isset( $params['overwriteDest'] ) ) {
95	95	$ok = unlink( $dest );
96	96	if ( !$ok ) {
97		~~- $status->fatal( "Could not delete destination file." );~~
	97	+ $status->fatal( 'backend-fail-delete', $params['dest'] );
98	98	return $status;
99	99	}
100	100	} elseif ( isset( $params['overwriteSame'] ) ) {
101	101	if ( !$this->filesAreSame( $source, $dest ) ) {
102		~~- $status->fatal( "Non-identical destination file already exists." );~~
	102	+ $status->fatal( 'backend-fail-notsame', $params['source'], $params['dest'] );
103	103	}
104	104	return $status; // do nothing; either OK or bad status
105	105	} else {
106		~~- $status->fatal( "Destination file already exists." );~~
	106	+ $status->fatal( 'backend-fail-alreadyexists', $params['dest'] );
107	107	return $status;
108	108	}
109	109	} else {
—	—	@@ -113,7 +113,7 @@
114	114	$ok = rename( $source, $dest );
115	115	wfRestoreWarnings();
116	116	if ( !$ok ) {
117		~~- $status->fatal( "Could not move file to destination." );~~
	117	+ $status->fatal( 'backend-fail-move', $params['source'], $params['dest'] );
118	118	return $status;
119	119	}
120	120
—	—	@@ -125,13 +125,13 @@
126	126
127	127	list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );
128	128	if ( $source === null ) {
129		~~- $status->fatal( "Invalid storage path {$params['source']}." );~~
	129	+ $status->fatal( 'backend-fail-invalidpath', $params['source'] );
130	130	return $status;
131	131	}
132	132
133	133	if ( !file_exists( $source ) ) {
134	134	if ( !$params['ignoreMissingSource'] ) {
135		~~- $status->fatal( "Could not delete source because it does not exist." );~~
	135	+ $status->fatal( 'backend-fail-delete', $params['source'] );
136	136	}
137	137	return $status; // do nothing; either OK or bad status
138	138	}
—	—	@@ -140,7 +140,7 @@
141	141	$ok = unlink( $source );
142	142	wfRestoreWarnings();
143	143	if ( !$ok ) {
144		~~- $status->fatal( "Could not delete source file." );~~
	144	+ $status->fatal( 'backend-fail-delete', $params['source'] );
145	145	return $status;
146	146	}
147	147
—	—	@@ -152,14 +152,14 @@
153	153
154	154	list( $c, $dest ) = $this->resolveVirtualPath( $params['dest'] );
155	155	if ( $dest === null ) {
156		~~- $status->fatal( "Invalid storage path {$params['dest']}." );~~
	156	+ $status->fatal( 'backend-fail-invalidpath', $params['dest'] );
157	157	return $status;
158	158	}
159	159
160	160	// Check if the destination file exists and we can't handle that
161	161	$destExists = file_exists( $dest );
162	162	if ( $destExists && !$params['overwriteDest'] && !$params['overwriteSame'] ) {
163		~~- $status->fatal( "Destination file already exists." ); // short-circuit~~
	163	+ $status->fatal( 'backend-fail-alreadyexists', $params['dest'] );
164	164	return $status;
165	165	}
166	166
—	—	@@ -168,7 +168,7 @@
169	169	$tmpPath = tempnam( wfTempDir(), 'file_concatenate' );
170	170	wfRestoreWarnings();
171	171	if ( $tmpPath === false ) {
172		~~- $status->fatal( "Could not create temporary file $tmpPath." );~~
	172	+ $status->fatal( 'backend-fail-createtemp' );
173	173	return $status;
174	174	}
175	175
—	—	@@ -177,30 +177,30 @@
178	178	$tmpHandle = fopen( $tmpPath, 'a' );
179	179	wfRestoreWarnings();
180	180	if ( $tmpHandle === false ) {
181		~~- $status->fatal( "Could not open temporary file $tmpPath." );~~
	181	+ $status->fatal( 'backend-fail-opentemp', $tmpPath );
182	182	return $status;
183	183	}
184	184	foreach ( $params['sources'] as $virtualSource ) {
185	185	list( $c, $source ) = $this->resolveVirtualPath( $virtualSource );
186	186	if ( $source === null ) {
187		~~- $status->fatal( "Invalid storage path {$virtualSource}." );~~
	187	+ $status->fatal( 'backend-fail-invalidpath', $virtualSource );
188	188	return $status;
189	189	}
190	190	// Load chunk into memory (it should be a small file)
191	191	$chunk = file_get_contents( $source );
192	192	if ( $chunk === false ) {
193		~~- $status->fatal( "Could not read source file $source." );~~
	193	+ $status->fatal( 'backend-fail-read', $virtualSource );
194	194	return $status;
195	195	}
196	196	// Append chunk to file (pass chunk size to avoid magic quotes)
197	197	if ( !fwrite( $tmpHandle, $chunk, count( $chunk ) ) ) {
198		~~- $status->fatal( "Could not write to temporary file $tmpPath." );~~
	198	+ $status->fatal( 'backend-fail-writetemp', $tmpPath );
199	199	return $status;
200	200	}
201	201	}
202	202	wfSuppressWarnings();
203	203	if ( !fclose( $tmpHandle ) ) {
204		~~- $status->fatal( "Could not close temporary file $tmpPath." );~~
	204	+ $status->fatal( 'backend-fail-closetemp', $tmpPath );
205	205	return $status;
206	206	}
207	207	wfRestoreWarnings();
—	—	@@ -213,12 +213,12 @@
214	214	$ok = unlink( $dest );
215	215	wfRestoreWarnings();
216	216	if ( !$ok ) {
217		~~- $status->fatal( "Could not delete destination file." );~~
	217	+ $status->fatal( 'backend-fail-delete', $params['dest'] );
218	218	return $status;
219	219	}
220	220	} elseif ( isset( $params['overwriteSame'] ) ) {
221	221	if ( !$this->filesAreSame( $tmpPath, $dest ) ) {
222		~~- $status->fatal( "Non-identical destination file already exists." );~~
	222	+ $status->fatal( 'backend-fail-notsame', $tmpPath, $params['dest'] );
223	223	}
224	224	return $status; // do nothing; either OK or bad status
225	225	}
—	—	@@ -232,7 +232,7 @@
233	233	$ok = rename( $tmpPath, $dest );
234	234	wfRestoreWarnings();
235	235	if ( !$ok ) {
236		~~- $status->fatal( "Could not rename temporary file to destination." );~~
	236	+ $status->fatal( 'backend-fail-move', $tmpPath, $params['dest'] );
237	237	return $status;
238	238	}
239	239
—	—	@@ -271,13 +271,13 @@
272	272
273	273	list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );
274	274	if ( $source === null ) {
275		~~- $status->fatal( "Invalid storage path {$params['source']}." );~~
	275	+ $status->fatal( 'backend-fail-invalidpath', $params['source'] );
276	276	return $status;
277	277	}
278	278
279	279	$ok = StreamFile::stream( $source, array(), false );
280	280	if ( !$ok ) {
281		~~- $status->fatal( "Unable to stream file {$source}." );~~
	281	+ $status->fatal( 'backend-fail-stream', $params['source'] );
282	282	return $status;
283	283	}
284	284
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileLockManager.php
—	—	@@ -1,74 +1,66 @@
2	2	<?php
3	3	/**
4	4	* FileBackend helper class for handling file locking.
5		~~- * Implemenations can rack of what locked in the process cache.~~
	5	+ * Locks on resource keys can either be shared or exclusive.
	6	+ *
	7	+ * Implemenations can keep track of what is locked in the process cache.
6	8	* This can reduce hits to external resources for lock()/unlock() calls.
7	9	*
8	10	* Subclasses should avoid throwing exceptions at all costs.
9	11	*/
10	12	abstract class FileLockManager {
	13	+ /* Lock types; stronger locks have high values */
	14	+ const LOCK_SH = 1; // shared lock (for reads)
	15	+ const LOCK_EX = 2; // exclusive lock (for writes)
	16	+
11	17	/**
12	18	* Construct a new instance from configuration
	19	+ *
13	20	* @param $config Array
14	21	*/
15		~~- abstract public function __construct( array $config );~~
	22	+ public function __construct( array $config ) {}
16	23
17	24	/**
18		~~- * Lock the files at a storage paths for a backend~~
	25	+ * Lock the resources at the given abstract paths
19	26	*
20		~~- * @param $backendKey string~~
21		~~- * @param $paths Array List of storage paths~~
	27	+ * @param $paths Array List of resource names
	28	+ * @param $type integer FileLockManager::LOCK_EX, FileLockManager::LOCK_SH
22	29	* @return Status
23	30	*/
24		~~- final public function lock( $backendKey, array $paths ) {~~
25		~~- $keys = array();~~
26		~~- foreach ( $paths as $path ) {~~
27		~~- $keys[] = $this->getLockName( $backendKey, $path );~~
28		~~- }~~
29		~~- return $this->doLock( $keys );~~
	31	+ final public function lock( array $paths, $type = self::LOCK_EX ) {
	32	+ $keys = array_map( 'sha1', $paths );
	33	+ return $this->doLock( $keys, $type );
30	34	}
31	35
32	36	/**
33		~~- * Unlock the files at a storage paths for a backend~~
	37	+ * Unlock the resources at the given abstract paths
34	38	*
35		~~- * @param $backendKey string~~
36	39	* @param $paths Array List of storage paths
37	40	* @return Status
38	41	*/
39		~~- final public function unlock( $backendKey, array $paths ) {~~
40		~~- $keys = array();~~
41		~~- foreach ( $paths as $path ) {~~
42		~~- $keys[] = $this->getLockName( $backendKey, $path );~~
43		~~- }~~
44		~~- return $this->doUnlock( $keys );~~
	42	+ final public function unlock( array $paths ) {
	43	+ $keys = array_map( 'sha1', $paths );
	44	+ return $this->doUnlock( $keys, 0 );
45	45	}
46	46
47	47	/**
48		~~- * Get the lock name given backend key (type/name) and a storage path~~
49		- *
50		~~- * @param $backendKey string~~
51		~~- * @param $name string~~
52		~~- * @return string~~
53		~~- */~~
54		~~- private function getLockName( $backendKey, $name ) {~~
55		~~- return urlencode( $backendKey ) . ':' . md5( $name );~~
56		~~- }~~
57		-
58		- /**
59	48	* Lock a resource with the given key
60	49	*
61		~~- * @param $key Array List of keys~~
	50	+ * @param $key Array List of keys to lock
	51	+ * @param $type integer FileLockManager::LOCK_EX, FileLockManager::LOCK_SH
62	52	* @return string
63	53	*/
64		~~- abstract protected function doLock( array $keys );~~
	54	+ abstract protected function doLock( array $keys, $type );
65	55
66	56	/**
67		~~- * Unlock a resource with the given key~~
	57	+ * Unlock a resource with the given key.
	58	+ * If $type is given, then only locks of that type should be cleared.
68	59	*
69		~~- * @param $key Array List of keys~~
	60	+ * @param $key Array List of keys to unlock
	61	+ * @param $type integer FileLockManager::LOCK_EX, FileLockManager::LOCK_SH, or 0
70	62	* @return string
71	63	*/
72		~~- abstract protected function doUnlock( array $keys );~~
	64	+ abstract protected function doUnlock( array $keys, $type );
73	65	}
74	66
75	67	/**
—	—	@@ -79,24 +71,29 @@
80	72	*/
81	73	class FSFileLockManager extends FileLockManager {
82	74	protected $lockDir; // global dir for all servers
83		~~- /** @var Array Map of lock key names to lock file handlers */~~
	75	+ /** @var Array Map of (locked key => lock type => lock file handle) */
84	76	protected $handles = array();
85	77
86	78	function __construct( array $config ) {
87	79	$this->lockDir = $config['lockDir'];
88	80	}
89	81
90		~~- function doLock( array $keys ) {~~
	82	+ function doLock( array $keys, $type ) {
91	83	$status = Status::newGood();
92	84
93	85	$lockedKeys = array(); // files locked in this attempt
94	86	foreach ( $keys as $key ) {
95		~~- $status->merge( $this->doSingleLock( $key ) );~~
96		~~- if ( $status->isOk() ) {~~
97		~~- $lockedKeys[] = $key;~~
	87	+ $subStatus = $this->doSingleLock( $key, $type );
	88	+ $status->merge( $subStatus );
	89	+ if ( $status->isOK() ) {
	90	+ // Don't append to $lockedKeys if $key is already locked.
	91	+ // We do NOT want to unlock the key if we have to rollback.
	92	+ if ( $subStatus->isGood() ) { // no warnings/fatals?
	93	+ $lockedKeys[] = $key;
	94	+ }
98	95	} else {
99	96	// Abort and unlock everything
100		~~- $status->merge( $this->doUnlock( $lockedKeys ) );~~
	97	+ $status->merge( $this->doUnlock( $lockedKeys, $type ) );
101	98	return $status;
102	99	}
103	100	}
—	—	@@ -104,33 +101,44 @@
105	102	return $status;
106	103	}
107	104
108		~~- function doUnlock( array $keys ) {~~
	105	+ function doUnlock( array $keys, $type ) {
109	106	$status = Status::newGood();
110	107
111	108	foreach ( $keys as $key ) {
112		~~- $status->merge( $this->doSingleUnlock( $key ) );~~
	109	+ $status->merge( $this->doSingleUnlock( $key, $type ) );
113	110	}
114	111
115	112	return $status;
116	113	}
117	114
118		~~- protected function doSingleLock( $key ) {~~
	115	+ /**
	116	+ * Lock a single resource key
	117	+ *
	118	+ * @param $key string
	119	+ * @param $type integer
	120	+ * @return Status
	121	+ */
	122	+ protected function doSingleLock( $key, $type ) {
119	123	$status = Status::newGood();
120	124
121		~~- if ( isset( $this->handles[$key] ) ) {~~
122		~~- $status->warning( 'File already locked.' );~~
	125	+ if ( isset( $this->handles[$key][$type] ) ) {
	126	+ $status->warning( 'lockmanager-alreadylocked', $key );
	127	+ } elseif ( isset( $this->handles[$key][self::LOCK_EX] ) ) {
	128	+ $status->warning( 'lockmanager-alreadylocked', $key );
123	129	} else {
124	130	wfSuppressWarnings();
125		~~- $handle = fopen( "{$this->lockDir}/{$key}", 'w' );~~
	131	+ $handle = fopen( "{$this->lockDir}/{$key}", 'c' );
126	132	if ( $handle ) {
127		~~- if ( flock( $handle, LOCK_SH ) ) {~~
128		~~- $this->handles[$key] = $handle;~~
	133	+ // Either a shared or exclusive lock
	134	+ $lock = ( $type == self::LOCK_SH ) ? LOCK_SH : LOCK_EX;
	135	+ if ( flock( $handle, $lock ) ) {
	136	+ $this->handles[$key][$type] = $handle;
129	137	} else {
130	138	fclose( $handle );
131		~~- $status->fatal( "Could not file acquire lock." );~~
	139	+ $status->fatal( 'lockmanager-fail-acquirelock', $key );
132	140	}
133	141	} else {
134		~~- $status->fatal( "Could not open lock file." );~~
	142	+ $status->fatal( 'lockmanager-fail-openlock', $key );
135	143	}
136	144	wfRestoreWarnings();
137	145	}
—	—	@@ -138,24 +146,43 @@
139	147	return $status;
140	148	}
141	149
142		~~- protected function doSingleUnlock( $key ) {~~
	150	+ /**
	151	+ * Unlock a single resource key
	152	+ *
	153	+ * @param $key string
	154	+ * @param $type integer
	155	+ * @return Status
	156	+ */
	157	+ protected function doSingleUnlock( $key, $type ) {
143	158	$status = Status::newGood();
144	159
145		~~- if ( isset( $this->handles[$key] ) ) {~~
146		~~- wfSuppressWarnings();~~
147		~~- if ( !flock( $this->handles[$key], LOCK_UN ) ) {~~
148		~~- $status->fatal( "Could not unlock file." );~~
	160	+ if ( !isset( $this->handles[$key] ) ) {
	161	+ $status->warning( 'lockmanager-notlocked', $key );
	162	+ } elseif ( $type && !isset( $this->handles[$key][$type] ) ) {
	163	+ $status->warning( 'lockmanager-notlocked', $key );
	164	+ } else {
	165	+ foreach ( $this->handles[$key] as $lockType => $handle ) {
	166	+ if ( $type && $lockType != $type ) {
	167	+ continue; // only unlock locks of type $type
	168	+ }
	169	+ wfSuppressWarnings();
	170	+ if ( !flock( $this->handles[$key][$lockType], LOCK_UN ) ) {
	171	+ $status->fatal( 'lockmanager-fail-releaselock', $key );
	172	+ }
	173	+ if ( !fclose( $this->handles[$key][$lockType] ) ) {
	174	+ $status->warning( 'lockmanager-fail-closelock', $key );
	175	+ }
	176	+ wfRestoreWarnings();
	177	+ unset( $this->handles[$key][$lockType] );
149	178	}
150		~~- if ( !fclose( $this->handles[$key] ) ) {~~
151		~~- $status->warning( "Could not close lock file." );~~
	179	+ if ( !count( $this->handles[$key] ) ) {
	180	+ wfSuppressWarnings();
	181	+ # No locks are held for the lock file anymore
	182	+ if ( !unlink( "{$this->lockDir}/{$key}" ) ) {
	183	+ $status->warning( 'lockmanager-fail-deletelock', $key );
	184	+ }
	185	+ wfRestoreWarnings();
152	186	}
153		~~- if ( !unlink( "{$this->lockDir}/{$key}" ) ) {~~
154		~~- $status->warning( "Could not delete lock file." );~~
155		~~- }~~
156		~~- wfRestoreWarnings();~~
157		~~- unset( $this->handles[$key] );~~
158		~~- } else {~~
159		~~- $status->warning( "There is no file lock to unlock." );~~
160	187	}
161	188
162	189	return $status;
—	—	@@ -163,9 +190,11 @@
164	191
165	192	protected function __destruct() {
166	193	// Make sure remaining files get cleared for sanity
167		~~- foreach ( $this->handles as $key => $handler ) {~~
168		~~- flock( $handler, LOCK_UN ); // PHP 5.3 will not do this automatically~~
169		~~- fclose( $handler );~~
	194	+ foreach ( $this->handles as $key => $locks ) {
	195	+ foreach ( $locks as $type => $handle ) {
	196	+ flock( $handle, LOCK_UN ); // PHP 5.3 will not do this automatically
	197	+ fclose( $handle );
	198	+ }
170	199	unlink( "{$this->lockDir}/{$key}" );
171	200	}
172	201	}
—	—	@@ -191,8 +220,8 @@
192	221	class DBFileLockManager extends FileLockManager {
193	222	/** @var Array Map of bucket indexes to server names */
194	223	protected $serverMap = array(); // (index => (server1,server2,...))
195		~~- /** @var Array List of active lock key names */~~
196		~~- protected $locksHeld = array(); // (key => 1)~~
	224	+ /** @var Array Map of (locked key => lock type => 1) */
	225	+ protected $locksHeld = array();
197	226	/** $var Array Map Lock-active database connections (name => Database) */
198	227	protected $activeConns = array();
199	228
—	—	@@ -221,18 +250,20 @@
222	251	}
223	252	}
224	253
225		~~- function doLock( array $keys ) {~~
	254	+ function doLock( array $keys, $type ) {
226	255	$status = Status::newGood();
227	256
228	257	$keysToLock = array();
229	258	// Get locks that need to be acquired (buckets => locks)...
230	259	foreach ( $keys as $key ) {
231		~~- if ( isset( $this->locksHeld[$key] ) ) {~~
232		~~- $status->warning( 'File already locked.' );~~
	260	+ if ( isset( $this->locksHeld[$key][$type] ) ) {
	261	+ $status->warning( 'lockmanager-alreadylocked', $key );
	262	+ } elseif ( isset( $this->locksHeld[$key][self::LOCK_EX] ) ) {
	263	+ $status->warning( 'lockmanager-alreadylocked', $key );
233	264	} else {
234	265	$bucket = $this->getBucketFromKey( $key );
235	266	if ( $bucket === null ) { // config error?
236		~~- $status->fatal( "Lock servers for key $key is not set." );~~
	267	+ $status->fatal( 'lockmanager-fail-config', $key );
237	268	return $status;
238	269	}
239	270	$keysToLock[$bucket][] = $key;
—	—	@@ -249,25 +280,26 @@
250	281	// (b) Server is down but a fallback is up
251	282	// (c) Server is down and no fallbacks are up (or none defined)
252	283	try {
253		~~- $this->lockingSelect( $server, $keys ); // SELECT FOR UPDATE~~
	284	+ $this->lockingSelect( $server, $keys, $type );
254	285	} catch ( DBError $e ) {
255	286	// Can we manage to lock on any of the fallback servers?
256		~~- if ( !$this->lockingSelectFallbacks( $bucket, $keys ) ) {~~
	287	+ if ( $this->lockingSelectFallbacks( $bucket, $keys, $type ) ) {
	288	+ // Recovered; a fallback server is up
	289	+ $propagateToFallbacks = false; // done already
	290	+ } else {
257	291	// Abort and unlock everything we just locked
258		~~- $status->fatal( "Could not contact the lock server." );~~
259		~~- $status->merge( $this->doUnlock( $lockedKeys ) );~~
	292	+ $status->fatal( 'lockmanager-fail-db', $bucket );
	293	+ $status->merge( $this->doUnlock( $lockedKeys, $type ) );
260	294	return $status;
261		~~- } else { // recovered using fallbacks~~
262		~~- $propagateToFallbacks = false; // done already~~
263	295	}
264	296	}
265	297	// Propagate any locks to the fallback servers (best effort)
266	298	if ( $propagateToFallbacks ) {
267		~~- $this->lockingSelectFallbacks( $bucket, $keys );~~
	299	+ $this->lockingSelectFallbacks( $bucket, $keys, $type );
268	300	}
269	301	// Record locks as active
270	302	foreach ( $keys as $key ) {
271		~~- $this->locksHeld[$key] = 1; // locked~~
	303	+ $this->locksHeld[$key][$type] = 1; // locked
272	304	}
273	305	// Keep track of what locks were made in this attempt
274	306	$lockedKeys = array_merge( $lockedKeys, $keys );
—	—	@@ -276,21 +308,29 @@
277	309	return $status;
278	310	}
279	311
280		~~- function doUnlock( array $keys ) {~~
	312	+ function doUnlock( array $keys, $type ) {
281	313	$status = Status::newGood();
282	314
283	315	foreach ( $keys as $key ) {
284		~~- if ( $this->locksHeld[$key] ) {~~
285		~~- unset( $this->locksHeld[$key] );~~
286		~~- // Reference count the locks held and COMMIT when zero~~
287		~~- if ( !count( $this->locksHeld ) ) {~~
288		~~- $this->commitLockTransactions();~~
	316	+ if ( !isset( $this->locksHeld[$key] ) ) {
	317	+ $status->warning( 'lockmanager-notlocked', $key );
	318	+ } elseif ( $type && !isset( $this->locksHeld[$key][$type] ) ) {
	319	+ $status->warning( 'lockmanager-notlocked', $key );
	320	+ } else {
	321	+ foreach ( $this->locksHeld[$key] as $lockType => $x ) {
	322	+ if ( $type && $lockType != $type ) {
	323	+ continue; // only unlock locks of type $type
	324	+ }
	325	+ unset( $this->locksHeld[$key][$lockType] );
289	326	}
290		~~- } else {~~
291		~~- $status->warning( "There is no file lock to unlock." );~~
292	327	}
293	328	}
294	329
	330	+ // Reference count the locks held and COMMIT when zero
	331	+ if ( !count( $this->locksHeld ) ) {
	332	+ $this->commitLockTransactions();
	333	+ }
	334	+
295	335	return $status;
296	336	}
297	337
—	—	@@ -299,19 +339,23 @@
300	340	*
301	341	* @param $server string
302	342	* @param $keys Array
	343	+ * @param $type integer FileLockManager::LOCK_EX or FileLockManager::LOCK_SH
303	344	* @return void
304	345	*/
305		~~- protected function lockingSelect( $server, array $keys ) {~~
	346	+ protected function lockingSelect( $server, array $keys, $type ) {
306	347	if ( !isset( $this->activeConns[$server] ) ) {
307	348	$this->activeConns[$server] = wfGetDB( DB_MASTER, array(), $server );
308	349	$this->activeConns[$server]->begin(); // start transaction
309	350	}
	351	+ $lockingClause = ( $type == self::LOCK_SH )
	352	+ ? 'LOCK IN SHARE MODE' // reader lock
	353	+ : 'FOR UPDATE'; // writer lock
310	354	$this->activeConns[$server]->select(
311	355	'file_locking',
312	356	'1',
313	357	array( 'fl_key' => $keys ),
314	358	__METHOD__,
315		~~- array( 'FOR UPDATE' )~~
	359	+ array( $lockingClause )
316	360	);
317	361	}
318	362
—	—	@@ -321,15 +365,16 @@
322	366	*
323	367	* @param $bucket integer
324	368	* @param $keys Array
	369	+ * @param $type integer FileLockManager::LOCK_EX or FileLockManager::LOCK_SH
325	370	* @return bool Locks made on at least one fallback server
326	371	*/
327		~~- protected function lockingSelectFallbacks( $bucket, array $keys ) {~~
	372	+ protected function lockingSelectFallbacks( $bucket, array $keys, $type ) {
328	373	$locksMade = false;
329		~~- $count = count( $this->serverMap[$bucket] );~~
330		~~- for ( $i=1; $i < $count; $i++ ) { // start at 1 to only include fallbacks~~
	374	+ // Start at $i=1 to only include fallback servers
	375	+ for ( $i=1; $i < count( $this->serverMap[$bucket] ); $i++ ) {
331	376	$server = $this->serverMap[$bucket][$i];
332	377	try {
333		~~- $this->doLockingSelect( $server, $keys ); // SELECT FOR UPDATE~~
	378	+ $this->doLockingSelect( $server, $keys, $type );
334	379	$locksMade = true; // success for this fallback
335	380	} catch ( DBError $e ) {
336	381	// oh well; best effort (@TODO: logging?)
—	—	@@ -380,11 +425,11 @@
381	426	class NullFileLockManager extends FileLockManager {
382	427	function __construct( array $config ) {}
383	428
384		~~- function doLock( array $keys ) {~~
	429	+ function doLock( array $keys, $type ) {
385	430	return Status::newGood();
386	431	}
387	432
388		~~- function doUnlock( array $keys ) {~~
	433	+ function doUnlock( array $keys, $type ) {
389	434	return Status::newGood();
390	435	}
391	436	}
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php
—	—	@@ -334,18 +334,27 @@
335	335	}
336	336
337	337	final public function lockFiles( array $paths ) {
338		~~- // Locks should be specific to this backend location~~
339		~~- $backendKey = get_class( $this ) . '-' . $this->getName();~~
340		~~- return $this->lockManager->lock( $backendKey, $paths ); // not supported~~
	338	+ return $this->lockManager->lock( $this->getLockResourcePaths( $paths ) );
341	339	}
342	340
343	341	final public function unlockFiles( array $paths ) {
344		~~- // Locks should be specific to this backend location~~
345		~~- $backendKey = get_class( $this ) . '-' . $this->getName();~~
346		~~- return $this->lockManager->unlock( $backendKey, $paths ); // not supported~~
	342	+ return $this->lockManager->unlock( $this->getLockResourcePaths( $paths ) );
347	343	}
348	344
349	345	/**
	346	+ * Get a prefix to use for locking keys
	347	+ * @return string
	348	+ */
	349	+ private function getLockResourcePaths( array $paths ) {
	350	+ $backendKey = get_class( $this ) . ':' . $this->getName();
	351	+ $res = array();
	352	+ foreach( $paths as $path ) {
	353	+ $res[] = "{$backendKey}:{$path}";
	354	+ }
	355	+ return $res;
	356	+ }
	357	+
	358	+ /**
350	359	* Split a storage path (e.g. "mwstore://container/path/to/object")
351	360	* into a container name and a full object name within that container.
352	361	*
—	—	@@ -554,7 +563,7 @@
555	564	// Create a temporary backup copy...
556	565	$this->tmpDestFile = $this->getLocalCopy( $this->params['dest'] );
557	566	if ( !$this->tmpDestFile ) {
558		~~- $status->fatal( "Could not backup destination file." );~~
	567	+ $status->fatal( 'backend-fail-restore', $this->params['dest'] );
559	568	return $status;
560	569	}
561	570	}
—	—	@@ -750,7 +759,7 @@
751	760	$status = Status::newGood();
752	761	if ( !$this->params['ignoreMissingSource'] ) {
753	762	if ( !$this->backend->fileExists( $this->params['source'] ) ) {
754		~~- $status->fatal( "Cannot delete file because it does not exist." );~~
	763	+ $status->fatal( 'backend-fail-notexists', $this->params['source'] );
755	764	return $status;
756	765	}
757	766	}

Status & tagging log

23:51, 7 December 2011 Siebrand (talk | contribs) changed the status of r103877 [removed: new added: deferred]