Tim Starling has submitted this change and it was merged.
Change subject: [LockManager] Various fixes to lock managers.
......................................................................
[LockManager] Various fixes to lock managers.
* Improved handling of corrupt values in cache for MemcLockManager.
Also improved the use of Status warnings a bit.
* Removed broken special-case handling for SH->EX lock escalation.
Updated MysqLockManager to compensate.
* Made FSLockManager only use one handle per file, which is more
efficient and avoids errors when escalating locks (SH->EX).
* Made lock unit tests have more useful output on failure.
Change-Id: Ib304712fa2b6b3fd02bfc1b08b6f238c771960c2
---
M includes/filebackend/lockmanager/DBLockManager.php
M includes/filebackend/lockmanager/FSLockManager.php
M includes/filebackend/lockmanager/MemcLockManager.php
M includes/filebackend/lockmanager/QuorumLockManager.php
M tests/phpunit/includes/filebackend/FileBackendTest.php
5 files changed, 148 insertions(+), 89 deletions(-)
Approvals:
Tim Starling: Verified; Looks good to me, approved
jenkins-bot: Checked
diff --git a/includes/filebackend/lockmanager/DBLockManager.php
b/includes/filebackend/lockmanager/DBLockManager.php
index ae33a24..f02387d 100644
--- a/includes/filebackend/lockmanager/DBLockManager.php
+++ b/includes/filebackend/lockmanager/DBLockManager.php
@@ -256,27 +256,38 @@
$status = Status::newGood();
$db = $this->getConnection( $lockSrv ); // checked in
isServerUp()
- $keys = array_unique( array_map( array( $this,
'sha1Base36Absolute' ), $paths ) );
+
+ $keys = array(); // list of hash keys for the paths
+ $data = array(); // list of rows to insert
+ $checkEXKeys = array(); // list of hash keys that this has no
EX lock on
# Build up values for INSERT clause
- $data = array();
- foreach ( $keys as $key ) {
+ foreach ( $paths as $path ) {
+ $key = $this->sha1Base36Absolute( $path );
+ $keys[] = $key;
$data[] = array( 'fls_key' => $key, 'fls_session' =>
$this->session );
+ if ( !isset( $this->locksHeld[$path][self::LOCK_EX] ) )
{
+ $checkEXKeys[] = $key;
+ }
}
- # Block new writers...
+
+ # Block new writers (both EX and SH locks leave entries here)...
$db->insert( 'filelocks_shared', $data, __METHOD__, array(
'IGNORE' ) );
# Actually do the locking queries...
if ( $type == self::LOCK_SH ) { // reader locks
+ $blocked = false;
# Bail if there are any existing writers...
- $blocked = $db->selectField( 'filelocks_exclusive', '1',
- array( 'fle_key' => $keys ),
- __METHOD__
- );
- # Prospective writers that haven't yet updated
filelocks_exclusive
- # will recheck filelocks_shared after doing so and bail
due to our entry.
+ if ( count( $checkEXKeys ) ) {
+ $blocked = $db->selectField(
'filelocks_exclusive', '1',
+ array( 'fle_key' => $checkEXKeys ),
+ __METHOD__
+ );
+ }
+ # Other prospective writers that haven't yet updated
filelocks_exclusive
+ # will recheck filelocks_shared after doing so and bail
due to this entry.
} else { // writer locks
$encSession = $db->addQuotes( $this->session );
# Bail if there are any existing writers...
- # The may detect readers, but the safe check for them
is below.
+ # This may detect readers, but the safe check for them
is below.
# Note: if two writers come at the same time, both bail
:)
$blocked = $db->selectField( 'filelocks_shared', '1',
array( 'fls_key' => $keys, "fls_session !=
$encSession" ),
diff --git a/includes/filebackend/lockmanager/FSLockManager.php
b/includes/filebackend/lockmanager/FSLockManager.php
index f374fdd..eacba70 100644
--- a/includes/filebackend/lockmanager/FSLockManager.php
+++ b/includes/filebackend/lockmanager/FSLockManager.php
@@ -43,7 +43,7 @@
protected $lockDir; // global dir for all servers
- /** @var Array Map of (locked key => lock type => lock file handle) */
+ /** @var Array Map of (locked key => lock file handle) */
protected $handles = array();
/**
@@ -115,12 +115,16 @@
} elseif ( isset( $this->locksHeld[$path][self::LOCK_EX] ) ) {
$this->locksHeld[$path][$type] = 1;
} else {
- wfSuppressWarnings();
- $handle = fopen( $this->getLockPath( $path ), 'a+' );
- wfRestoreWarnings();
- if ( !$handle ) { // lock dir missing?
- wfMkdirParents( $this->lockDir );
- $handle = fopen( $this->getLockPath( $path ),
'a+' ); // try again
+ if ( isset( $this->handles[$path] ) ) {
+ $handle = $this->handles[$path];
+ } else {
+ wfSuppressWarnings();
+ $handle = fopen( $this->getLockPath( $path ),
'a+' );
+ wfRestoreWarnings();
+ if ( !$handle ) { // lock dir missing?
+ wfMkdirParents( $this->lockDir );
+ $handle = fopen( $this->getLockPath(
$path ), 'a+' ); // try again
+ }
}
if ( $handle ) {
// Either a shared or exclusive lock
@@ -128,7 +132,7 @@
if ( flock( $handle, $lock | LOCK_NB ) ) {
// Record this lock as active
$this->locksHeld[$path][$type] = 1;
- $this->handles[$path][$type] = $handle;
+ $this->handles[$path] = $handle;
} else {
fclose( $handle );
$status->fatal(
'lockmanager-fail-acquirelock', $path );
@@ -160,24 +164,13 @@
--$this->locksHeld[$path][$type];
if ( $this->locksHeld[$path][$type] <= 0 ) {
unset( $this->locksHeld[$path][$type] );
- // If a LOCK_SH comes in while we have a
LOCK_EX, we don't
- // actually add a handler, so check for handler
existence.
- if ( isset( $this->handles[$path][$type] ) ) {
- if ( $type === self::LOCK_EX
- && isset(
$this->locksHeld[$path][self::LOCK_SH] )
- && !isset(
$this->handles[$path][self::LOCK_SH] ) )
- {
- // EX lock came first: move
this handle to the SH one
-
$this->handles[$path][self::LOCK_SH] = $this->handles[$path][$type];
- } else {
- // Mark this handle to be
unlocked and closed
- $handlesToClose[] =
$this->handles[$path][$type];
- }
- unset( $this->handles[$path][$type] );
- }
}
if ( !count( $this->locksHeld[$path] ) ) {
unset( $this->locksHeld[$path] ); // no locks
on this path
+ if ( isset( $this->handles[$path] ) ) {
+ $handlesToClose[] =
$this->handles[$path];
+ unset( $this->handles[$path] );
+ }
}
// Unlock handles to release locks and delete
// any lock files that end up with no locks on them...
diff --git a/includes/filebackend/lockmanager/MemcLockManager.php
b/includes/filebackend/lockmanager/MemcLockManager.php
index 6eb8b85..8131f81 100644
--- a/includes/filebackend/lockmanager/MemcLockManager.php
+++ b/includes/filebackend/lockmanager/MemcLockManager.php
@@ -28,8 +28,8 @@
* This is meant for multi-wiki systems that may share files.
* All locks are non-blocking, which avoids deadlocks.
*
- * All lock requests for a resource, identified by a hash string, will map
- * to one bucket. Each bucket maps to one or several peer servers, each
running memcached.
+ * All lock requests for a resource, identified by a hash string, will map to
one
+ * bucket. Each bucket maps to one or several peer servers, each running
memcached.
* A majority of peers must agree for a lock to be acquired.
*
* @ingroup LockManager
@@ -49,7 +49,7 @@
protected $serversUp = array(); // (server name => bool)
protected $lockExpiry; // integer; maximum time locks can be held
- protected $session = ''; // string; random SHA-1 UUID
+ protected $session = ''; // string; random UUID
/**
* Construct a new instance from configuration.
@@ -118,8 +118,8 @@
foreach ( $paths as $path ) {
$locksKey = $this->recordKeyForPath( $path );
$locksHeld = isset( $lockRecords[$locksKey] )
- ? $lockRecords[$locksKey]
- : array( self::LOCK_SH => array(),
self::LOCK_EX => array() ); // init
+ ? self::sanitizeLockArray(
$lockRecords[$locksKey] )
+ : self::newLockArray(); // init
foreach ( $locksHeld[self::LOCK_EX] as $session =>
$expiry ) {
if ( $expiry < $now ) { // stale?
unset(
$locksHeld[self::LOCK_EX][$session] );
@@ -146,9 +146,15 @@
// If there were no lock conflicts, update all the lock
records...
if ( $status->isOK() ) {
- foreach ( $lockRecords as $locksKey => $locksHeld ) {
- $memc->set( $locksKey, $locksHeld );
- wfDebug( __METHOD__ . ": acquired lock on key
$locksKey.\n" );
+ foreach ( $paths as $path ) {
+ $locksKey = $this->recordKeyForPath( $path );
+ $locksHeld = $lockRecords[$locksKey];
+ $ok = $memc->set( $locksKey, $locksHeld,
7*86400 );
+ if ( !$ok ) {
+ $status->fatal(
'lockmanager-fail-acquirelock', $path );
+ } else {
+ wfDebug( __METHOD__ . ": acquired lock
on key $locksKey.\n" );
+ }
}
}
@@ -183,17 +189,22 @@
foreach ( $paths as $path ) {
$locksKey = $this->recordKeyForPath( $path ); // lock
record
if ( !isset( $lockRecords[$locksKey] ) ) {
+ $status->warning(
'lockmanager-fail-releaselock', $path );
continue; // nothing to do
}
- $locksHeld = $lockRecords[$locksKey];
- if ( is_array( $locksHeld ) && isset( $locksHeld[$type]
) ) {
- unset( $locksHeld[$type][$this->session] );
- $ok = $memc->set( $locksKey, $locksHeld );
+ $locksHeld = self::sanitizeLockArray(
$lockRecords[$locksKey] );
+ if ( isset( $locksHeld[$type][$this->session] ) ) {
+ unset( $locksHeld[$type][$this->session] ); //
unregister this session
+ if ( $locksHeld === self::newLockArray() ) {
+ $ok = $memc->delete( $locksKey );
+ } else {
+ $ok = $memc->set( $locksKey, $locksHeld
);
+ }
+ if ( !$ok ) {
+ $status->fatal(
'lockmanager-fail-releaselock', $path );
+ }
} else {
- $ok = true;
- }
- if ( !$ok ) {
- $status->fatal( 'lockmanager-fail-releaselock',
$path );
+ $status->warning(
'lockmanager-fail-releaselock', $path );
}
wfDebug( __METHOD__ . ": released lock on key
$locksKey.\n" );
}
@@ -252,6 +263,26 @@
}
/**
+ * @return Array An empty lock structure for a key
+ */
+ protected static function newLockArray() {
+ return array( self::LOCK_SH => array(), self::LOCK_EX =>
array() );
+ }
+
+ /**
+ * @param $a array
+ * @return Array An empty lock structure for a key
+ */
+ protected static function sanitizeLockArray( $a ) {
+ if ( is_array( $a ) && isset( $a[self::LOCK_EX] ) && isset(
$a[self::LOCK_SH] ) ) {
+ return $a;
+ } else {
+ trigger_error( __METHOD__ . ": reset invalid lock
array.", E_USER_WARNING );
+ return self::newLockArray();
+ }
+ }
+
+ /**
* @param $memc MemcachedBagOStuff
* @param array $keys List of keys to acquire
* @return bool
@@ -279,10 +310,10 @@
continue; // acquire in order
}
}
- } while ( count( $lockedKeys ) < count( $keys ) && ( microtime(
true ) - $start ) <= 6 );
+ } while ( count( $lockedKeys ) < count( $keys ) && ( microtime(
true ) - $start ) <= 3 );
if ( count( $lockedKeys ) != count( $keys ) ) {
- $this->releaseMutexes( $lockedKeys ); // failed;
release what was locked
+ $this->releaseMutexes( $memc, $lockedKeys ); // failed;
release what was locked
return false;
}
diff --git a/includes/filebackend/lockmanager/QuorumLockManager.php
b/includes/filebackend/lockmanager/QuorumLockManager.php
index 010a38d..b331b54 100644
--- a/includes/filebackend/lockmanager/QuorumLockManager.php
+++ b/includes/filebackend/lockmanager/QuorumLockManager.php
@@ -46,8 +46,6 @@
foreach ( $paths as $path ) {
if ( isset( $this->locksHeld[$path][$type] ) ) {
++$this->locksHeld[$path][$type];
- } elseif ( isset(
$this->locksHeld[$path][self::LOCK_EX] ) ) {
- $this->locksHeld[$path][$type] = 1;
} else {
$bucket = $this->getBucketFromPath( $path );
$pathsToLock[$bucket][] = $path;
diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php
b/tests/phpunit/includes/filebackend/FileBackendTest.php
index a08910a..b7cf446 100644
--- a/tests/phpunit/includes/filebackend/FileBackendTest.php
+++ b/tests/phpunit/includes/filebackend/FileBackendTest.php
@@ -2024,53 +2024,79 @@
private function doTestLockCalls() {
$backendName = $this->backendClass();
- for ( $i=0; $i<50; $i++ ) {
- $paths = array(
- "test1.txt",
- "test2.txt",
- "test3.txt",
- "subdir1",
- "subdir1", // duplicate
- "subdir1/test1.txt",
- "subdir1/test2.txt",
- "subdir2",
- "subdir2", // duplicate
- "subdir2/test3.txt",
- "subdir2/test4.txt",
- "subdir2/subdir",
- "subdir2/subdir/test1.txt",
- "subdir2/subdir/test2.txt",
- "subdir2/subdir/test3.txt",
- "subdir2/subdir/test4.txt",
- "subdir2/subdir/test5.txt",
- "subdir2/subdir/sub",
- "subdir2/subdir/sub/test0.txt",
- "subdir2/subdir/sub/120-px-file.txt",
- );
+ $paths = array(
+ "test1.txt",
+ "test2.txt",
+ "test3.txt",
+ "subdir1",
+ "subdir1", // duplicate
+ "subdir1/test1.txt",
+ "subdir1/test2.txt",
+ "subdir2",
+ "subdir2", // duplicate
+ "subdir2/test3.txt",
+ "subdir2/test4.txt",
+ "subdir2/subdir",
+ "subdir2/subdir/test1.txt",
+ "subdir2/subdir/test2.txt",
+ "subdir2/subdir/test3.txt",
+ "subdir2/subdir/test4.txt",
+ "subdir2/subdir/test5.txt",
+ "subdir2/subdir/sub",
+ "subdir2/subdir/sub/test0.txt",
+ "subdir2/subdir/sub/120-px-file.txt",
+ );
+ for ( $i=0; $i<25; $i++ ) {
$status = $this->backend->lockFiles( $paths,
LockManager::LOCK_EX );
- $this->assertEquals( array(), $status->errors,
- "Locking of files succeeded ($backendName)." );
+ $this->assertEquals( print_r( array(), true ), print_r(
$status->errors, true ),
+ "Locking of files succeeded ($backendName)
($i)." );
$this->assertEquals( true, $status->isOK(),
- "Locking of files succeeded with OK status
($backendName)." );
+ "Locking of files succeeded with OK status
($backendName) ($i)." );
$status = $this->backend->lockFiles( $paths,
LockManager::LOCK_SH );
- $this->assertEquals( array(), $status->errors,
- "Locking of files succeeded ($backendName)." );
+ $this->assertEquals( print_r( array(), true ), print_r(
$status->errors, true ),
+ "Locking of files succeeded ($backendName)
($i)." );
$this->assertEquals( true, $status->isOK(),
- "Locking of files succeeded with OK status
($backendName)." );
+ "Locking of files succeeded with OK status
($backendName) ($i)." );
$status = $this->backend->unlockFiles( $paths,
LockManager::LOCK_SH );
- $this->assertEquals( array(), $status->errors,
- "Locking of files succeeded ($backendName)." );
+ $this->assertEquals( print_r( array(), true ), print_r(
$status->errors, true ),
+ "Locking of files succeeded ($backendName)
($i)." );
$this->assertEquals( true, $status->isOK(),
- "Locking of files succeeded with OK status
($backendName)." );
+ "Locking of files succeeded with OK status
($backendName) ($i)." );
$status = $this->backend->unlockFiles( $paths,
LockManager::LOCK_EX );
- $this->assertEquals( array(), $status->errors,
- "Locking of files succeeded ($backendName)." );
+ $this->assertEquals( print_r( array(), true ), print_r(
$status->errors, true ),
+ "Locking of files succeeded ($backendName).
($i)" );
$this->assertEquals( true, $status->isOK(),
- "Locking of files succeeded with OK status
($backendName)." );
+ "Locking of files succeeded with OK status
($backendName) ($i)." );
+
+ ## Flip the acquire/release ordering around ##
+
+ $status = $this->backend->lockFiles( $paths,
LockManager::LOCK_SH );
+ $this->assertEquals( print_r( array(), true ), print_r(
$status->errors, true ),
+ "Locking of files succeeded ($backendName)
($i)." );
+ $this->assertEquals( true, $status->isOK(),
+ "Locking of files succeeded with OK status
($backendName) ($i)." );
+
+ $status = $this->backend->lockFiles( $paths,
LockManager::LOCK_EX );
+ $this->assertEquals( print_r( array(), true ), print_r(
$status->errors, true ),
+ "Locking of files succeeded ($backendName)
($i)." );
+ $this->assertEquals( true, $status->isOK(),
+ "Locking of files succeeded with OK status
($backendName) ($i)." );
+
+ $status = $this->backend->unlockFiles( $paths,
LockManager::LOCK_EX );
+ $this->assertEquals( print_r( array(), true ), print_r(
$status->errors, true ),
+ "Locking of files succeeded ($backendName).
($i)" );
+ $this->assertEquals( true, $status->isOK(),
+ "Locking of files succeeded with OK status
($backendName) ($i)." );
+
+ $status = $this->backend->unlockFiles( $paths,
LockManager::LOCK_SH );
+ $this->assertEquals( print_r( array(), true ), print_r(
$status->errors, true ),
+ "Locking of files succeeded ($backendName)
($i)." );
+ $this->assertEquals( true, $status->isOK(),
+ "Locking of files succeeded with OK status
($backendName) ($i)." );
}
$status = Status::newGood();
--
To view, visit https://gerrit.wikimedia.org/r/49926
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib304712fa2b6b3fd02bfc1b08b6f238c771960c2
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Demon <ch...@wikimedia.org>
Gerrit-Reviewer: Tim Starling <tstarl...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
