Merge pull request #5605 from magento-performance/MC-33363

[2.4] Second part delivery of Redis improvement delivery - MC-33363

Author: vzabaznov

app/code/Magento/Catalog/Model/Product.php

@@ -73,9 +73,9 @@ class Product extends \Magento\Catalog\Model\AbstractModel implements
     const STORE_ID = 'store_id';
 
     /**
-     * @var string
+     * @var string|bool
      */
-    protected $_cacheTag = self::CACHE_TAG;
+    protected $_cacheTag = false;
 
     /**
      * @var string
@@ -878,7 +878,6 @@ public function getAttributes($groupId = null, $skipSuper = false)
      */
     public function beforeSave()
     {
-        $this->cleanCache();
         $this->setTypeHasOptions(false);
         $this->setTypeHasRequiredOptions(false);
         $this->setHasOptions(false);

app/code/Magento/Config/App/Config/Type/System.php

@@ -20,6 +20,8 @@
 use Magento\Store\Model\Config\Processor\Fallback;
 use Magento\Framework\Encryption\Encryptor;
 use Magento\Store\Model\ScopeInterface as StoreScope;
+use Magento\Framework\App\Cache\StateInterface;
+use Magento\Framework\App\Cache\Type\Config;
 
 /**
  * System configuration type
@@ -98,6 +100,12 @@ class System implements ConfigTypeInterface
     private $lockQuery;
 
     /**
+     * @var StateInterface
+     */
+    private $cacheState;
+
+    /**
+     * System constructor.
      * @param ConfigSourceInterface $source
      * @param PostProcessorInterface $postProcessor
      * @param Fallback $fallback
@@ -110,6 +118,7 @@ class System implements ConfigTypeInterface
      * @param Encryptor|null $encryptor
      * @param LockManagerInterface|null $locker
      * @param LockGuardedCacheLoader|null $lockQuery
+     * @param StateInterface|null $cacheState
      * @SuppressWarnings(PHPMD.UnusedFormalParameter)
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      */
@@ -125,7 +134,8 @@ public function __construct(
         Reader $reader = null,
         Encryptor $encryptor = null,
         LockManagerInterface $locker = null,
-        LockGuardedCacheLoader $lockQuery = null
+        LockGuardedCacheLoader $lockQuery = null,
+        StateInterface $cacheState = null
     ) {
         $this->postProcessor = $postProcessor;
         $this->cache = $cache;
@@ -136,6 +146,8 @@ public function __construct(
             ?: ObjectManager::getInstance()->get(Encryptor::class);
         $this->lockQuery = $lockQuery
             ?: ObjectManager::getInstance()->get(LockGuardedCacheLoader::class);
+        $this->cacheState = $cacheState
+            ?: ObjectManager::getInstance()->get(StateInterface::class);
     }
 
     /**
@@ -220,6 +232,10 @@ private function getWithParts($path)
      */
     private function loadAllData()
     {
+        if (!$this->cacheState->isEnabled(Config::TYPE_IDENTIFIER)) {
+            return $this->readData();
+        }
+
         $loadAction = function () {
             $cachedData = $this->cache->load($this->configType);
             $data = false;
@@ -245,6 +261,10 @@ private function loadAllData()
      */
     private function loadDefaultScopeData($scopeType)
     {
+        if (!$this->cacheState->isEnabled(Config::TYPE_IDENTIFIER)) {
+            return $this->readData();
+        }
+
         $loadAction = function () use ($scopeType) {
             $cachedData = $this->cache->load($this->configType . '_' . $scopeType);
             $scopeData = false;
@@ -271,6 +291,10 @@ private function loadDefaultScopeData($scopeType)
      */
     private function loadScopeData($scopeType, $scopeId)
     {
+        if (!$this->cacheState->isEnabled(Config::TYPE_IDENTIFIER)) {
+            return $this->readData();
+        }
+
         $loadAction = function () use ($scopeType, $scopeId) {
             $cachedData = $this->cache->load($this->configType . '_' . $scopeType . '_' . $scopeId);
             $scopeData = false;
@@ -393,6 +417,10 @@ public function clean()
             $this->cache->clean(\Zend_Cache::CLEANING_MODE_MATCHING_TAG, [self::CACHE_TAG]);
         };
 
+        if (!$this->cacheState->isEnabled(Config::TYPE_IDENTIFIER)) {
+            return $cleanAction();
+        }
+
         $this->lockQuery->lockedCleanData(
             self::$lockName,
             $cleanAction

app/code/Magento/Config/etc/di.xml

@@ -97,8 +97,6 @@
     <virtualType name="systemConfigQueryLocker" type="Magento\Framework\Cache\LockGuardedCacheLoader">
         <arguments>
             <argument name="locker" xsi:type="object">Magento\Framework\Lock\Backend\Cache</argument>
-            <argument name="lockTimeout" xsi:type="number">42000</argument>
-            <argument name="delayTimeout" xsi:type="number">100</argument>
         </arguments>
     </virtualType>
 

app/etc/di.xml

@@ -1787,8 +1787,6 @@
     <type name="Magento\Framework\Cache\LockGuardedCacheLoader">
         <arguments>
             <argument name="locker" xsi:type="object">Magento\Framework\Lock\Backend\Cache</argument>
-            <argument name="lockTimeout" xsi:type="number">10000</argument>
-            <argument name="delayTimeout" xsi:type="number">20</argument>
         </arguments>
     </type>
     <preference for="Magento\Framework\HTTP\AsyncClientInterface" type="Magento\Framework\HTTP\AsyncClient\GuzzleAsyncClient" />

lib/internal/Magento/Framework/Cache/LockGuardedCacheLoader.php

@@ -48,22 +48,31 @@ class LockGuardedCacheLoader
     private $loadTimeout;
 
     /**
-     * LockGuardedCacheLoader constructor.
+     * Minimal delay timeout in ms.
+     *
+     * @var int
+     */
+    private $minimalDelayTimeout;
+
+    /**
      * @param LockManagerInterface $locker
      * @param int $lockTimeout
      * @param int $delayTimeout
      * @param int $loadTimeout
+     * @param int $minimalDelayTimeout
      */
     public function __construct(
         LockManagerInterface $locker,
         int $lockTimeout = 10000,
         int $delayTimeout = 20,
-        int $loadTimeout = 10000
+        int $loadTimeout = 10000,
+        int $minimalDelayTimeout = 5
     ) {
         $this->locker = $locker;
         $this->lockTimeout = $lockTimeout;
         $this->delayTimeout = $delayTimeout;
         $this->loadTimeout = $loadTimeout;
+        $this->minimalDelayTimeout = $minimalDelayTimeout;
     }
 
     /**
@@ -82,7 +91,7 @@ public function lockedLoadData(
         callable $dataSaver
     ) {
         $cachedData = $dataLoader(); //optimistic read
-        $deadline = microtime(true) + $this->loadTimeout;
+        $deadline = microtime(true) + $this->loadTimeout / 100;
 
         while ($cachedData === false) {
             if ($deadline <= microtime(true)) {
@@ -100,7 +109,7 @@ public function lockedLoadData(
             }
 
             if ($cachedData === false) {
-                usleep($this->delayTimeout * 1000);
+                usleep($this->getLookupTimeout() * 1000);
                 $cachedData = $dataLoader();
             }
         }
@@ -118,14 +127,21 @@ public function lockedLoadData(
     public function lockedCleanData(string $lockName, callable $dataCleaner)
     {
         while ($this->locker->isLocked($lockName)) {
-            usleep($this->delayTimeout * 1000);
-        }
-        try {
-            if ($this->locker->lock($lockName, $this->lockTimeout / 1000)) {
-                $dataCleaner();
-            }
-        } finally {
-            $this->locker->unlock($lockName);
+            usleep($this->getLookupTimeout() * 1000);
         }
+
+        $dataCleaner();
+    }
+
+    /**
+     * Delay will be applied as rand($minimalDelayTimeout, $delayTimeout).
+     * This helps to desynchronize multiple clients trying
+     * to acquire the lock for the same resource at the same time
+     *
+     * @return int
+     */
+    private function getLookupTimeout()
+    {
+        return rand($this->minimalDelayTimeout, $this->delayTimeout);
     }
 }

lib/internal/Magento/Framework/Lock/Backend/Cache.php

@@ -24,32 +24,69 @@ class Cache implements \Magento\Framework\Lock\LockManagerInterface
      */
     private $cache;
 
+    /**
+     * Sign for locks, helps to avoid removing a lock that was created by another client
+     *
+     * @string
+     */
+    private $lockSign;
+
     /**
      * @param FrontendInterface $cache
      */
     public function __construct(FrontendInterface $cache)
     {
         $this->cache = $cache;
+        $this->lockSign = $this->generateLockSign();
     }
 
     /**
      * @inheritdoc
      */
     public function lock(string $name, int $timeout = -1): bool
     {
-        if ((bool)$this->cache->test($this->getIdentifier($name))) {
+        if (empty($this->lockSign)) {
+            $this->lockSign = $this->generateLockSign();
+        }
+
+        $data = $this->cache->load($this->getIdentifier($name));
+
+        if (false !== $data) {
              return false;
         }
 
-        return $this->cache->save('1', $this->getIdentifier($name), [], $timeout);
+        $this->cache->save($this->lockSign, $this->getIdentifier($name), [], $timeout * 100);
+
+        $data = $this->cache->load($this->getIdentifier($name));
+
+        if ($data === $this->lockSign) {
+            return true;
+        }
+
+        return false;
     }
 
     /**
      * @inheritdoc
      */
     public function unlock(string $name): bool
     {
-        return (bool)$this->cache->remove($this->getIdentifier($name));
+        if (empty($this->lockSign)) {
+            return false;
+        }
+
+        $data = $this->cache->load($this->getIdentifier($name));
+
+        if (false === $data) {
+            return false;
+        }
+
+        $removeResult = false;
+        if ($data === $this->lockSign) {
+            $removeResult = (bool)$this->cache->remove($this->getIdentifier($name));
+        }
+
+        return $removeResult;
     }
 
     /**
@@ -70,4 +107,27 @@ private function getIdentifier(string $cacheIdentifier): string
     {
         return self::LOCK_PREFIX . $cacheIdentifier;
     }
+
+    /**
+     * Function that generates lock sign that helps to avoid removing a lock that was created by another client.
+     *
+     * @return string
+     */
+    private function generateLockSign()
+    {
+        $sign = implode(
+            '-',
+            [
+                \getmypid(), \crc32(\gethostname())
+            ]
+        );
+
+        try {
+            $sign .= '-' . \bin2hex(\random_bytes(4));
+        } catch (\Exception $e) {
+            $sign .= '-' . \uniqid('-uniqid-');
+        }
+
+        return $sign;
+    }
 }

lib/internal/Magento/Framework/Lock/Test/Unit/Backend/CacheTest.php

@@ -53,6 +53,16 @@ public function testUnlock(): void
     {
         $identifier = 'lock_name';
 
+        $closure = \Closure::bind(function ($cacheInstance) {
+            return $cacheInstance->lockSign;
+        }, null, $this->cache);
+        $lockSign = $closure($this->cache);
+
+        $this->frontendCacheMock
+            ->expects($this->once())->method('load')
+            ->with(self::LOCK_PREFIX . $identifier)
+            ->willReturn($lockSign);
+
         $this->frontendCacheMock
             ->expects($this->once())
             ->method('remove')
@@ -61,4 +71,22 @@ public function testUnlock(): void
 
         $this->assertEquals(true, $this->cache->unlock($identifier));
     }
+
+    /**
+     * Verify that lock will no be released without sign matches.
+     * Sign generates in Cache class constructor.
+     *
+     * @return void
+     */
+    public function testUnlockWithAnotherSign(): void
+    {
+        $identifier = 'lock_name';
+
+        $this->frontendCacheMock
+            ->expects($this->once())->method('load')
+            ->with(self::LOCK_PREFIX . $identifier)
+            ->willReturn(\uniqid('some_rand-'));
+
+        $this->assertEquals(false, $this->cache->unlock($identifier));
+    }
 }

setup/src/Magento/Setup/Model/ConfigOptionsList/Cache.php

@@ -20,7 +20,7 @@
 class Cache implements ConfigOptionsListInterface
 {
     const INPUT_VALUE_CACHE_REDIS = 'redis';
-    const CONFIG_VALUE_CACHE_REDIS = 'Cm_Cache_Backend_Redis';
+    const CONFIG_VALUE_CACHE_REDIS = '\\Magento\\Framework\\Cache\\Backend\Redis';
 
     const INPUT_KEY_CACHE_BACKEND = 'cache-backend';
     const INPUT_KEY_CACHE_BACKEND_REDIS_SERVER = 'cache-backend-redis-server';
@@ -233,7 +233,7 @@ private function validateRedisConfig(array $options, DeploymentConfig $deploymen
                 self::CONFIG_PATH_CACHE_BACKEND_DATABASE,
                 $this->getDefaultConfigValue(self::INPUT_KEY_CACHE_BACKEND_REDIS_DATABASE)
             );
-        
+
         $config['password'] = isset($options[self::INPUT_KEY_CACHE_BACKEND_REDIS_PASSWORD])
             ? $options[self::INPUT_KEY_CACHE_BACKEND_REDIS_PASSWORD]
             : $deploymentConfig->get(
@@ -282,6 +282,6 @@ private function getDefaultConfigValue($inputKey)
      */
     private function generateCachePrefix(): string
     {
-        return substr(\md5(dirname(__DIR__, 6)), 0, 3) . '_';
+        return substr(\hash('sha256', dirname(__DIR__, 6)), 0, 3) . '_';
     }
 }

setup/src/Magento/Setup/Model/ConfigOptionsList/PageCache.php

@@ -20,7 +20,7 @@
 class PageCache implements ConfigOptionsListInterface
 {
     const INPUT_VALUE_PAGE_CACHE_REDIS = 'redis';
-    const CONFIG_VALUE_PAGE_CACHE_REDIS = 'Cm_Cache_Backend_Redis';
+    const CONFIG_VALUE_PAGE_CACHE_REDIS = '\\Magento\\Framework\\Cache\\Backend\Redis';
 
     const INPUT_KEY_PAGE_CACHE_BACKEND = 'page-cache';
     const INPUT_KEY_PAGE_CACHE_BACKEND_REDIS_SERVER = 'page-cache-redis-server';
@@ -284,6 +284,6 @@ private function getDefaultConfigValue($inputKey)
      */
     private function generateCachePrefix(): string
     {
-        return substr(\md5(dirname(__DIR__, 6)), 0, 3) . '_';
+        return substr(\hash('sha256', dirname(__DIR__, 6)), 0, 3) . '_';
     }
 }