[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

diff --git a/includes/filerepo/backend/lockmanager/FSLockManager.php b/includes/filerepo/backend/lockmanager/FSLockManager.php
index 27c65ec..21a3a4e 100644 (file)
--- 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 af88bc8..9c36a57 100644 (file)
--- 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 ) );
                        }
                }
        }