From a9e028e5a8d907a376e494f542cb89e69a129637 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 10 Apr 2012 21:56:42 -0700 Subject: [PATCH] [FileBackend] File locking fixes. * Fixed unlocking logic in FSLockManager for case when an EX lock was made, then an SH one, and then the EX one was "unlocked" * Avoid hiding useful unlink() warnings in FSLockManager * Reduced locking use in test cleanup code * Added a simple testLockCalls() test function * Made a few cleanups & fixes to backend tests Change-Id: I1110d9b537c450d9feca5a2fb35519c22435e81d --- .../backend/lockmanager/FSLockManager.php | 26 ++++--- .../includes/filerepo/FileBackendTest.php | 78 ++++++++++++++++--- 2 files changed, 84 insertions(+), 20 deletions(-) diff --git a/includes/filerepo/backend/lockmanager/FSLockManager.php b/includes/filerepo/backend/lockmanager/FSLockManager.php index 27c65ece7c..21a3a4ed9c 100644 --- a/includes/filerepo/backend/lockmanager/FSLockManager.php +++ b/includes/filerepo/backend/lockmanager/FSLockManager.php @@ -48,7 +48,7 @@ class FSLockManager extends LockManager { /** * Construct a new instance from configuration. - * + * * $config includes: * 'lockDirectory' : Directory containing the lock files * @@ -101,7 +101,7 @@ class FSLockManager extends LockManager { * * @param $path string * @param $type integer - * @return Status + * @return Status */ protected function doSingleLock( $path, $type ) { $status = Status::newGood(); @@ -139,10 +139,10 @@ class FSLockManager extends LockManager { /** * Unlock a single resource key - * + * * @param $path string * @param $type integer - * @return Status + * @return Status */ protected function doSingleUnlock( $path, $type ) { $status = Status::newGood(); @@ -159,8 +159,16 @@ class FSLockManager extends LockManager { // 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] ) ) { - // Mark this handle to be unlocked and closed - $handlesToClose[] = $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] ); } } @@ -172,7 +180,7 @@ class FSLockManager extends LockManager { $status->merge( $this->closeLockHandles( $path, $handlesToClose ) ); $status->merge( $this->pruneKeyLockFiles( $path ) ); } else { - // Unix: unlink() can be used on files currently open by this + // Unix: unlink() can be used on files currently open by this // process and we must do so in order to avoid race conditions $status->merge( $this->pruneKeyLockFiles( $path ) ); $status->merge( $this->closeLockHandles( $path, $handlesToClose ) ); @@ -185,14 +193,12 @@ class FSLockManager extends LockManager { private function closeLockHandles( $path, array $handlesToClose ) { $status = Status::newGood(); foreach ( $handlesToClose as $handle ) { - wfSuppressWarnings(); if ( !flock( $handle, LOCK_UN ) ) { $status->fatal( 'lockmanager-fail-releaselock', $path ); } if ( !fclose( $handle ) ) { $status->warning( 'lockmanager-fail-closelock', $path ); } - wfRestoreWarnings(); } return $status; } @@ -200,12 +206,10 @@ class FSLockManager extends LockManager { private function pruneKeyLockFiles( $path ) { $status = Status::newGood(); if ( !count( $this->locksHeld[$path] ) ) { - wfSuppressWarnings(); # No locks are held for the lock file anymore if ( !unlink( $this->getLockPath( $path ) ) ) { $status->warning( 'lockmanager-fail-deletelock', $path ); } - wfRestoreWarnings(); unset( $this->locksHeld[$path] ); unset( $this->handles[$path] ); } diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php index af88bc8ced..9c36a577bc 100644 --- a/tests/phpunit/includes/filerepo/FileBackendTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendTest.php @@ -205,7 +205,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->tearDownFiles(); } - function doTestStore( $op ) { + private function doTestStore( $op ) { $backendName = $this->backendClass(); $source = $op['src']; @@ -287,7 +287,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->tearDownFiles(); } - function doTestCopy( $op ) { + private function doTestCopy( $op ) { $backendName = $this->backendClass(); $source = $op['src']; @@ -668,7 +668,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->tearDownFiles(); } - public function doTestConcatenate( $params, $srcs, $srcsContent, $alreadyExists, $okStatus ) { + private function doTestConcatenate( $params, $srcs, $srcsContent, $alreadyExists, $okStatus ) { $backendName = $this->backendClass(); $expContent = ''; @@ -857,7 +857,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->tearDownFiles(); } - public function doTestGetFileContents( $source, $content ) { + private function doTestGetFileContents( $source, $content ) { $backendName = $this->backendClass(); $this->prepare( array( 'dir' => dirname( $source ) ) ); @@ -902,7 +902,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->tearDownFiles(); } - public function doTestGetLocalCopy( $source, $content ) { + private function doTestGetLocalCopy( $source, $content ) { $backendName = $this->backendClass(); $this->prepare( array( 'dir' => dirname( $source ) ) ); @@ -996,7 +996,7 @@ class FileBackendTest extends MediaWikiTestCase { ); } - function doTestPrepareAndClean( $path, $isOK ) { + private function doTestPrepareAndClean( $path, $isOK ) { $backendName = $this->backendClass(); $status = $this->prepare( array( 'dir' => dirname( $path ) ) ); @@ -1100,7 +1100,7 @@ class FileBackendTest extends MediaWikiTestCase { // @TODO: test some cases where the ops should fail } - function doTestDoOperations() { + private function doTestDoOperations() { $base = $this->baseStorePath(); $fileA = "$base/unittest-cont1/a/b/fileA.txt"; @@ -1117,6 +1117,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->backend->create( array( 'dst' => $fileB, 'content' => $fileBContents ) ); $this->prepare( array( 'dir' => dirname( $fileC ) ) ); $this->backend->create( array( 'dst' => $fileC, 'content' => $fileCContents ) ); + $this->prepare( array( 'dir' => dirname( $fileD ) ) ); $status = $this->backend->doOperations( array( array( 'op' => 'copy', 'src' => $fileA, 'dst' => $fileC, 'overwrite' => 1 ), @@ -1172,7 +1173,7 @@ class FileBackendTest extends MediaWikiTestCase { "Correct file SHA-1 of $fileC" ); } - function doTestDoOperationsFailing() { + private function doTestDoOperationsFailing() { $base = $this->baseStorePath(); $fileA = "$base/unittest-cont2/a/b/fileA.txt"; @@ -1563,6 +1564,64 @@ class FileBackendTest extends MediaWikiTestCase { foreach ( $iter as $iter ) {} // no errors } + public function testLockCalls() { + $this->backend = $this->singleBackend; + $this->doTestLockCalls(); + } + + 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", + ); + + $status = $this->backend->lockFiles( $paths, LockManager::LOCK_EX ); + $this->assertEquals( array(), $status->errors, + "Locking of files succeeded ($backendName)." ); + $this->assertEquals( true, $status->isOK(), + "Locking of files succeeded with OK status ($backendName)." ); + + $status = $this->backend->lockFiles( $paths, LockManager::LOCK_SH ); + $this->assertEquals( array(), $status->errors, + "Locking of files succeeded ($backendName)." ); + $this->assertEquals( true, $status->isOK(), + "Locking of files succeeded with OK status ($backendName)." ); + + $status = $this->backend->unlockFiles( $paths, LockManager::LOCK_SH ); + $this->assertEquals( array(), $status->errors, + "Locking of files succeeded ($backendName)." ); + $this->assertEquals( true, $status->isOK(), + "Locking of files succeeded with OK status ($backendName)." ); + + $status = $this->backend->unlockFiles( $paths, LockManager::LOCK_EX ); + $this->assertEquals( array(), $status->errors, + "Locking of files succeeded ($backendName)." ); + $this->assertEquals( true, $status->isOK(), + "Locking of files succeeded with OK status ($backendName)." ); + } + } + // test helper wrapper for backend prepare() function private function prepare( array $params ) { $this->dirsToPrune[] = $params['dir']; @@ -1588,7 +1647,8 @@ class FileBackendTest extends MediaWikiTestCase { $iter = $this->backend->getFileList( array( 'dir' => "$base/$container" ) ); if ( $iter ) { foreach ( $iter as $file ) { - $this->backend->delete( array( 'src' => "$base/$container/$file" ), array( 'force' => 1 ) ); + $this->backend->delete( array( 'src' => "$base/$container/$file" ), + array( 'force' => 1, 'nonLocking' => 1 ) ); } } } -- 2.20.1