[BUGFIX] Multiple fixes for Locking API and TSFE locking 40/38840/13
authorMarkus Klein <klein.t3@reelworx.at>
Tue, 21 Apr 2015 00:33:04 +0000 (02:33 +0200)
committerMarkus Klein <klein.t3@reelworx.at>
Mon, 27 Apr 2015 17:10:40 +0000 (19:10 +0200)
* Retrieve correct LockingStrategy for requested capabilities
* Prefix lock filenames to make them better visible in the folder
* Make all LockStrategies destroyable
* Semaphore locking now uses ftok() to generate a unique id
* Make the Mbox lock independent of the target file
* Introduce an access lock for each of the TSFE cache locks

We decrease the priority of Semaphore locking since this can
be pretty dangerous for the average user. If something goes
really wrong in the webserver (which is out of our control),
we might leave behind stale semaphores, which might cause
a permanent deadlock for an instance, which can only be resolved
by a server admin.
We might raise the priority again at a later point in time,
when we can provide better means of cleanup.

The new access locks protects the access to the cache locks in TSFE
now, which allows us to safely remove those cache locks after using
them. This way we don't spam the system with loads of locks.

Releases: master
Resolves: #66503
Change-Id: Ia19e6e7d47d7941e01785f5a6b67746a6c0fa368
Reviewed-on: http://review.typo3.org/38840
Reviewed-by: Andreas Allacher <andreas.allacher@gmx.at>
Tested-by: Andreas Allacher <andreas.allacher@gmx.at>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Markus Klein <klein.t3@reelworx.at>
Tested-by: Markus Klein <klein.t3@reelworx.at>
typo3/sysext/core/Classes/Locking/FileLockStrategy.php
typo3/sysext/core/Classes/Locking/LockFactory.php
typo3/sysext/core/Classes/Locking/LockingStrategyInterface.php
typo3/sysext/core/Classes/Locking/SemaphoreLockStrategy.php
typo3/sysext/core/Classes/Locking/SimpleLockStrategy.php
typo3/sysext/core/Classes/Mail/MboxTransport.php
typo3/sysext/core/Tests/Unit/Locking/FileLockStrategyTest.php
typo3/sysext/core/Tests/Unit/Locking/Fixtures/DummyLock.php
typo3/sysext/core/Tests/Unit/Locking/LockFactoryTest.php
typo3/sysext/core/Tests/Unit/Locking/SimpleLockStrategyTest.php
typo3/sysext/frontend/Classes/Controller/TypoScriptFrontendController.php

index 14c9c01..f61eb00 100644 (file)
@@ -65,7 +65,7 @@ class FileLockStrategy implements LockingStrategyInterface {
                if (!is_writable($path)) {
                        throw new LockCreateException('Cannot write to directory ' . $path, 1396278700);
                }
-               $this->filePath = $path . md5((string)$subject);
+               $this->filePath = $path . 'flock_' . md5((string)$subject);
        }
 
        /**
@@ -142,7 +142,7 @@ class FileLockStrategy implements LockingStrategyInterface {
         * @return int Returns a priority for the method. 0 to 100, 100 is highest
         */
        static public function getPriority() {
-               return 50;
+               return 75;
        }
 
        /**
@@ -150,7 +150,7 @@ class FileLockStrategy implements LockingStrategyInterface {
         */
        static public function getCapabilities() {
                if (PHP_SAPI === 'isapi') {
-                       // From php docs: When using a multithreaded server API like ISAPI you may not be able to rely on flock()
+                       // From php docs: When using a multi-threaded server API like ISAPI you may not be able to rely on flock()
                        // to protect files against other PHP scripts running in parallel threads of the same server instance!
                        return 0;
                }
@@ -158,4 +158,12 @@ class FileLockStrategy implements LockingStrategyInterface {
                return $capabilities;
        }
 
+       /**
+        * Destroys the resource associated with the lock
+        *
+        * @return void
+        */
+       public function destroy() {
+               @unlink($this->filePath);
+       }
 }
index 4d699e3..1161aa1 100644 (file)
@@ -30,7 +30,7 @@ class LockFactory implements SingletonInterface {
        protected $lockingStrategy = array(
                SemaphoreLockStrategy::class => TRUE,
                FileLockStrategy::class => TRUE,
-               SemaphoreLockStrategy::class => TRUE
+               SimpleLockStrategy::class => TRUE
        );
 
        /**
@@ -70,7 +70,8 @@ class LockFactory implements SingletonInterface {
 
                /** @var LockingStrategyInterface $method */
                foreach ($this->lockingStrategy as $method => $_) {
-                       if ($capabilities & $method::getCapabilities()) {
+                       $supportedCapabilities = $capabilities & $method::getCapabilities();
+                       if ($supportedCapabilities === $capabilities) {
                                $queue->insert($method, $method::getPriority());
                        }
                }
index c0bbd9f..390f23d 100644 (file)
@@ -74,6 +74,13 @@ interface LockingStrategyInterface {
        public function release();
 
        /**
+        * Destroys the resource associated with the lock
+        *
+        * @return void
+        */
+       public function destroy();
+
+       /**
         * Get status of this lock
         *
         * @return bool Returns TRUE if lock is acquired by this locker, FALSE otherwise
index 9229865..2c67ee6 100644 (file)
@@ -15,6 +15,8 @@ namespace TYPO3\CMS\Core\Locking;
  */
 
 use TYPO3\CMS\Core\Locking\Exception\LockAcquireException;
+use TYPO3\CMS\Core\Locking\Exception\LockCreateException;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
 
 /**
  * Semaphore locking
@@ -23,6 +25,8 @@ use TYPO3\CMS\Core\Locking\Exception\LockAcquireException;
  */
 class SemaphoreLockStrategy implements LockingStrategyInterface {
 
+       const FILE_LOCK_FOLDER = 'typo3temp/locks/';
+
        /**
         * @var mixed Identifier used for this lock
         */
@@ -34,15 +38,38 @@ class SemaphoreLockStrategy implements LockingStrategyInterface {
        protected $resource;
 
        /**
+        * @var string
+        */
+       protected $filePath = '';
+
+       /**
         * @var bool TRUE if lock is acquired
         */
        protected $isAcquired = FALSE;
 
        /**
         * @param string $subject ID to identify this lock in the system
+        * @throws LockCreateException
         */
        public function __construct($subject) {
-               $this->id = abs(crc32((string)$subject));
+               $path = PATH_site . self::FILE_LOCK_FOLDER;
+               if (!is_dir($path)) {
+                       // Not using mkdir_deep on purpose here, if typo3temp itself
+                       // does not exist, this issue should be solved on a different
+                       // level of the application.
+                       if (!GeneralUtility::mkdir($path)) {
+                               throw new LockCreateException('Cannot create directory ' . $path, 1395140007);
+                       }
+               }
+               if (!is_writable($path)) {
+                       throw new LockCreateException('Cannot write to directory ' . $path, 1396278700);
+               }
+               $this->filePath = $path  . 'sem_' . md5((string)$subject);
+               touch($this->filePath);
+               $this->id = ftok($this->filePath, 'A');
+               if ($this->id === FALSE) {
+                       throw new LockCreateException('Cannot create key for semaphore using path ' . $this->filePath, 1396278734);
+               }
        }
 
        /**
@@ -52,7 +79,9 @@ class SemaphoreLockStrategy implements LockingStrategyInterface {
                $this->release();
                // We do not call sem_remove() since this would remove the resource for other processes,
                // we leave that to the system. This is not clean, but there's no other way to determine when
-               // a semaphore is no longer needed.
+               // a semaphore is no longer needed as a website is generally running endlessly
+               // and we have no way to detect if there is a process currently waiting on that lock
+               // or if the server is shutdown
        }
 
        /**
@@ -112,7 +141,19 @@ class SemaphoreLockStrategy implements LockingStrategyInterface {
         * @return int Returns a priority for the method. 0 to 100, 100 is highest
         */
        static public function getPriority() {
-               return 75;
+               return 25;
+       }
+
+       /**
+        * Destroys the resource associated with the lock
+        *
+        * @return void
+        */
+       public function destroy() {
+               if ($this->resource) {
+                       sem_remove($this->resource);
+                       @unlink($this->filePath);
+               }
        }
 
 }
index d108e77..811e50a 100644 (file)
@@ -67,7 +67,7 @@ class SimpleLockStrategy implements LockingStrategyInterface {
                if (!is_writable($path)) {
                        throw new LockCreateException('Cannot write to directory ' . $path, 1396278700);
                }
-               $this->filePath = $path . md5((string)$subject);
+               $this->filePath = $path . 'simple_' . md5((string)$subject);
        }
 
        /**
@@ -133,7 +133,7 @@ class SimpleLockStrategy implements LockingStrategyInterface {
         *
         * @param int $mode LOCK_CAPABILITY_EXCLUSIVE or self::LOCK_CAPABILITY_NOBLOCK
         * @return bool Returns TRUE if the lock was acquired successfully
-        * @throws \RuntimeException with code 1428700748 if the acquire would have blocked and NOBLOCK was set
+        * @throws LockAcquireWouldBlockException
         */
        public function acquire($mode = self::LOCK_CAPABILITY_EXCLUSIVE) {
                if ($this->isAcquired) {
@@ -177,7 +177,15 @@ class SimpleLockStrategy implements LockingStrategyInterface {
         * @return int Returns a priority for the method. 0 to 100, 100 is highest
         */
        static public function getPriority() {
-               return 25;
+               return 50;
        }
 
+       /**
+        * Destroys the resource associated with the lock
+        *
+        * @return void
+        */
+       public function destroy() {
+               @unlink($this->filePath);
+       }
 }
index 897d750..de44f01 100644 (file)
@@ -80,7 +80,7 @@ class MboxTransport implements \Swift_Transport {
                $messageStr .= $message->toString();
                $messageStr .= LF . LF;
                $lockFactory = GeneralUtility::makeInstance(LockFactory::class);
-               $lockObject = $lockFactory->createLocker($this->debugFile);
+               $lockObject = $lockFactory->createLocker('mbox');
                $lockObject->acquire();
                // Write the mbox file
                $file = @fopen($this->debugFile, 'a');
@@ -92,7 +92,7 @@ class MboxTransport implements \Swift_Transport {
                @fclose($file);
                GeneralUtility::fixPermissions($this->debugFile);
                $lockObject->release();
-               // Return every receipient as "delivered"
+               // Return every recipient as "delivered"
                $count = count((array)$message->getTo()) + count((array)$message->getCc()) + count((array)$message->getBcc());
                return $count;
        }
index 5637430..35e7b87 100644 (file)
@@ -34,4 +34,12 @@ class FileLockStrategyTest extends UnitTestCase {
                $this->assertTrue(is_dir(PATH_site . FileLockStrategy::FILE_LOCK_FOLDER));
        }
 
+       /**
+        * @test
+        */
+       public function constructorSetsFilePathToExpectedValue() {
+               $lock = $this->getAccessibleMock(FileLockStrategy::class, ['dummy'], ['999999999']);
+               $this->assertSame(PATH_site . FileLockStrategy::FILE_LOCK_FOLDER  . 'flock_' . md5('999999999'), $lock->_get('filePath'));
+       }
+
 }
index 8b1e55b..3457513 100644 (file)
@@ -71,4 +71,11 @@ class DummyLock implements LockingStrategyInterface {
                return FALSE;
        }
 
+       /**
+        * Destroys the resource associated with the lock
+        *
+        * @return void
+        */
+       public function destroy() {
+       }
 }
index c42f1bc..65b59f3 100644 (file)
@@ -61,7 +61,7 @@ class LockFactoryTest extends UnitTestCase {
         * @test
         */
        public function getLockerReturnsExpectedClass() {
-               $this->mockFactory->_set('lockingStrategy', [FileLockStrategy::class => TRUE]);
+               $this->mockFactory->_set('lockingStrategy', [FileLockStrategy::class => TRUE, DummyLock::class => TRUE]);
                $locker = $this->mockFactory->createLocker('id', LockingStrategyInterface::LOCK_CAPABILITY_EXCLUSIVE | LockingStrategyInterface::LOCK_CAPABILITY_SHARED);
                $this->assertInstanceOf(FileLockStrategy::class, $locker);
        }
index 25a38c9..a754ce8 100644 (file)
@@ -39,7 +39,7 @@ class SimpleLockStrategyTest extends UnitTestCase {
         */
        public function constructorSetsResourceToPathWithIdIfUsingSimpleLocking() {
                $lock = $this->getAccessibleMock(SimpleLockStrategy::class, ['dummy'], ['999999999']);
-               $this->assertSame(PATH_site . SimpleLockStrategy::FILE_LOCK_FOLDER . md5('999999999'), $lock->_get('filePath'));
+               $this->assertSame(PATH_site . SimpleLockStrategy::FILE_LOCK_FOLDER  . 'simple_' . md5('999999999'), $lock->_get('filePath'));
        }
 
        /**
index 4b779af..b789c1f 100644 (file)
@@ -21,6 +21,7 @@ use TYPO3\CMS\Core\Charset\CharsetConverter;
 use TYPO3\CMS\Core\Error\Http\PageNotFoundException;
 use TYPO3\CMS\Core\Error\Http\ServiceUnavailableException;
 use TYPO3\CMS\Core\Localization\Locales;
+use TYPO3\CMS\Core\Locking\Exception\LockAcquireWouldBlockException;
 use TYPO3\CMS\Core\Locking\Locker;
 use TYPO3\CMS\Core\Messaging\ErrorpageMessage;
 use TYPO3\CMS\Core\Page\PageRenderer;
@@ -786,18 +787,9 @@ class TypoScriptFrontendController {
        protected $languageDependencies = array();
 
        /**
-        * Locking object for accessing "cache_pagesection"
-        *
-        * @var LockingStrategyInterface
-        */
-       public $pagesection_lockObj;
-
-       /**
-        * Locking object for accessing "cache_pages"
-        *
-        * @var LockingStrategyInterface
+        * @var LockingStrategyInterface[][]
         */
-       public $pages_lockObj;
+       protected $locks = [];
 
        /**
         * @var PageRenderer
@@ -2260,25 +2252,26 @@ class TypoScriptFrontendController {
                        return;
                }
 
-               $this->pagesection_lockObj = NULL;
                $pageSectionCacheContent = $this->tmpl->getCurrentPageData();
                if (!is_array($pageSectionCacheContent)) {
-                       // nothing in the cache, we acquire an exclusive lock now
-                       $key = $this->id . '::' . $this->MP;
-                       $lockFactory = GeneralUtility::makeInstance(LockFactory::class);
-                       $this->pagesection_lockObj = $lockFactory->createLocker($key);
-                       if (!$this->pagesection_lockObj->acquire()) {
-                               throw new \RuntimeException('Could not acquire lock for page section generation.', 1294586098);
-                       }
-                       // query the cache again to see if the data are there meanwhile
+                       // Nothing in the cache, we acquire an "exclusive lock" for the key now.
+                       // We use the Registry to store this lock centrally,
+                       // but we protect the access again with a global exclusive lock to avoid race conditions
+
+                       $this->acquireLock('pagesection', $this->id . '::' . $this->MP);
+                       //
+                       // from this point on we're the only one working on that page ($key)
+                       //
+
+                       // query the cache again to see if the page data are there meanwhile
                        $pageSectionCacheContent = $this->tmpl->getCurrentPageData();
                        if (is_array($pageSectionCacheContent)) {
-                               // we have the content, nice that some other process did the work for us
-                               $this->pagesection_lockObj->release();
-                               $this->pagesection_lockObj = NULL;
+                               // we have the content, nice that some other process did the work for us already
+                               $this->releaseLock('pagesection');
                        } else {
-                               // we keep the lock set, because we are the ones generating the page now and filling the cache
-                               // the lock will be released in releaseLocks()
+                               // We keep the lock set, because we are the ones generating the page now
+                               // and filling the cache.
+                               // This indicates that we have to release the lock in the Registry later in releaseLocks()
                        }
                }
 
@@ -2293,7 +2286,6 @@ class TypoScriptFrontendController {
                unset($pageSectionCacheContent);
 
                // Look for page in cache only if a shift-reload is not sent to the server.
-               $this->pages_lockObj = NULL;
                $lockHash = $this->getLockHash();
                if (!$this->headerNoCache()) {
                        if ($this->all) {
@@ -2303,20 +2295,21 @@ class TypoScriptFrontendController {
                                $row = $this->getFromCache_queryRow();
                                if (!is_array($row)) {
                                        // nothing in the cache, we acquire an exclusive lock now
-                                       $lockFactory = GeneralUtility::makeInstance(LockFactory::class);
-                                       $this->pages_lockObj = $lockFactory->createLocker($lockHash);
-                                       if (!$this->pages_lockObj->acquire()) {
-                                               throw new \RuntimeException('Could not acquire lock for page content generation.', 1294586099);
-                                       }
+
+                                       $this->acquireLock('pages', $lockHash);
+                                       //
+                                       // from this point on we're the only one working on that page ($lockHash)
+                                       //
+
                                        // query the cache again to see if the data are there meanwhile
                                        $row = $this->getFromCache_queryRow();
                                        if (is_array($row)) {
                                                // we have the content, nice that some other process did the work for us
-                                               $this->pages_lockObj->release();
-                                               $this->pages_lockObj = NULL;
+                                               $this->releaseLock('pages');
                                        } else {
-                                               // we keep the lock set, because we are the ones generating the page now and filling the cache
-                                               // the lock will be released in releaseLocks()
+                                               // We keep the lock set, because we are the ones generating the page now
+                                               // and filling the cache.
+                                               // This indicates that we have to release the lock in the Registry later in releaseLocks()
                                        }
                                }
                                if (is_array($row)) {
@@ -2363,11 +2356,7 @@ class TypoScriptFrontendController {
                }
                // the user forced rebuilding the page cache or there was no pagesection information
                // get a lock for the page content so other processes will not interrupt the regeneration
-               $lockFactory = GeneralUtility::makeInstance(LockFactory::class);
-               $this->pages_lockObj = $lockFactory->createLocker($lockHash);
-               if (!$this->pages_lockObj->acquire()) {
-                       throw new \RuntimeException('Could not acquire lock for page content generation.', 1294586100);
-               }
+               $this->acquireLock('pages', $lockHash);
        }
 
        /**
@@ -3271,14 +3260,8 @@ class TypoScriptFrontendController {
         * @return void
         */
        public function releaseLocks() {
-               if ($this->pagesection_lockObj) {
-                       $this->pagesection_lockObj->release();
-                       $this->pagesection_lockObj = NULL;
-               }
-               if ($this->pages_lockObj) {
-                       $this->pages_lockObj->release();
-                       $this->pages_lockObj = NULL;
-               }
+               $this->releaseLock('pagesection');
+               $this->releaseLock('pages');
        }
 
        /**
@@ -3308,18 +3291,15 @@ class TypoScriptFrontendController {
                $this->newHash = $this->getHash();
 
                // If the pages_lock is set, we are in charge of generating the page.
-               if (is_object($this->pages_lockObj)) {
+               if (is_object($this->locks['pages']['accessLock'])) {
                        // Here we put some temporary stuff in the cache in order to let the first hit generate the page.
                        // The temporary cache will expire after a few seconds (typ. 30) or will be cleared by the rendered page,
                        // which will also clear and rewrite the cache.
                        $this->tempPageCacheContent();
                }
                // At this point we have a valid pagesection_cache and also some temporary page_cache content,
-               // so let all other processes proceed now. (They are blocked at the pagessection_lock in getFromCaceh())
-               if ($this->pagesection_lockObj) {
-                       $this->pagesection_lockObj->release();
-                       $this->pagesection_lockObj = NULL;
-               }
+               // so let all other processes proceed now. (They are blocked at the pagessection_lock in getFromCache())
+               $this->releaseLock('pagesection');
 
                // Setting cache_timeout_default. May be overridden by PHP include scripts.
                $this->cacheTimeOutDefault = (int)$this->config['config']['cache_period'];
@@ -4732,4 +4712,74 @@ class TypoScriptFrontendController {
        public function getRequestedId() {
                return $this->requestedId ?: $this->id;
        }
+
+       /**
+        * Acquire a page specific lock
+        *
+        * @param string $type
+        * @param string $key
+        * @throws \InvalidArgumentException
+        * @throws \RuntimeException
+        * @throws \TYPO3\CMS\Core\Cache\Exception\NoSuchCacheException
+        */
+       protected function acquireLock($type, $key) {
+               $lockFactory = GeneralUtility::makeInstance(LockFactory::class);
+               $this->locks[$type]['accessLock'] = $lockFactory->createLocker($type);
+
+               $this->locks[$type]['pageLock'] = $lockFactory->createLocker(
+                       $key,
+                       LockingStrategyInterface::LOCK_CAPABILITY_EXCLUSIVE | LockingStrategyInterface::LOCK_CAPABILITY_NOBLOCK
+               );
+
+               do {
+                       if (!$this->locks[$type]['accessLock']->acquire()) {
+                               throw new \RuntimeException('Could not acquire access lock for "' . $type . '"".', 1294586098);
+                       }
+
+                       try {
+                               $locked = $this->locks[$type]['pageLock']->acquire(
+                                       LockingStrategyInterface::LOCK_CAPABILITY_EXCLUSIVE | LockingStrategyInterface::LOCK_CAPABILITY_NOBLOCK
+                               );
+                       } catch (LockAcquireWouldBlockException $e) {
+                               // somebody else has the lock, we keep waiting
+
+                               // first release the access lock
+                               $this->locks[$type]['accessLock']->release();
+                               // now lets make a short break (100ms) until we try again, since
+                               // the page generation by the lock owner will take a while anyways
+                               usleep(100000);
+                               continue;
+                       }
+                       $this->locks[$type]['accessLock']->release();
+                       if ($locked) {
+                               break;
+                       } else {
+                               throw new \RuntimeException('Could not acquire page lock for ' . $key . '.', 1294586098);
+                       }
+               } while (TRUE);
+       }
+
+       /**
+        * Release a page specific lock
+        *
+        * @param string $type
+        * @throws \InvalidArgumentException
+        * @throws \RuntimeException
+        * @throws \TYPO3\CMS\Core\Cache\Exception\NoSuchCacheException
+        */
+       protected function releaseLock($type) {
+               if ($this->locks[$type]['accessLock']) {
+                       if (!$this->locks[$type]['accessLock']->acquire()) {
+                               throw new \RuntimeException('Could not acquire access lock for "' . $type . '"".', 1294586098);
+                       }
+
+                       $this->locks[$type]['pageLock']->release();
+                       $this->locks[$type]['pageLock']->destroy();
+                       $this->locks[$type]['pageLock'] = NULL;
+
+                       $this->locks[$type]['accessLock']->release();
+                       $this->locks[$type]['accessLock'] = NULL;
+               }
+       }
+
 }