From 2135b80fcdb17bbde11e62b95ed8386b9c3d69f2 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 19 Feb 2013 10:51:44 -0800 Subject: [PATCH] [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 --- .../filebackend/lockmanager/DBLockManager.php | 33 ++++--- .../filebackend/lockmanager/FSLockManager.php | 39 ++++---- .../lockmanager/MemcLockManager.php | 67 +++++++++---- .../lockmanager/QuorumLockManager.php | 2 - .../includes/filebackend/FileBackendTest.php | 96 ++++++++++++------- 5 files changed, 148 insertions(+), 89 deletions(-) diff --git a/includes/filebackend/lockmanager/DBLockManager.php b/includes/filebackend/lockmanager/DBLockManager.php index ae33a247a9..f02387dc5b 100644 --- a/includes/filebackend/lockmanager/DBLockManager.php +++ b/includes/filebackend/lockmanager/DBLockManager.php @@ -256,27 +256,38 @@ class MySqlLockManager extends DBLockManager { $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 f374fdd193..eacba7042e 100644 --- a/includes/filebackend/lockmanager/FSLockManager.php +++ b/includes/filebackend/lockmanager/FSLockManager.php @@ -43,7 +43,7 @@ class FSLockManager extends LockManager { 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 @@ class FSLockManager extends LockManager { } 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 @@ class FSLockManager extends LockManager { 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 @@ class FSLockManager extends LockManager { --$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 6eb8b85b10..8131f81782 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 @@ class MemcLockManager extends QuorumLockManager { 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 @@ class MemcLockManager extends QuorumLockManager { 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 @@ class MemcLockManager extends QuorumLockManager { // 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 @@ class MemcLockManager extends QuorumLockManager { 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" ); } @@ -251,6 +262,26 @@ class MemcLockManager extends QuorumLockManager { return implode( ':', array( __CLASS__, 'locks', $this->sha1Base36Absolute( $path ) ) ); } + /** + * @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 @@ -279,10 +310,10 @@ class MemcLockManager extends QuorumLockManager { 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 010a38dd19..b331b540fc 100644 --- a/includes/filebackend/lockmanager/QuorumLockManager.php +++ b/includes/filebackend/lockmanager/QuorumLockManager.php @@ -46,8 +46,6 @@ abstract class QuorumLockManager extends LockManager { 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 a08910a92a..b7cf446111 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -2024,53 +2024,79 @@ class FileBackendTest extends MediaWikiTestCase { 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) ($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)." ); + "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( 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) ($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)." ); + "Locking of files succeeded with OK status ($backendName) ($i)." ); } $status = Status::newGood(); -- 2.20.1