Lock SCSS so we only run 1 job at a time

This is bit hacky but a start to lock the SCSS compiler properly
Retry during 10s then give up
Properly get error message
Do not clear locks and properly debug scss caching

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

diff --git a/lib/private/Template/IconsCacher.php b/lib/private/Template/IconsCacher.php
index dd83ce7877506da4f1b2687e370ea31a25d72664..3c0a270d3f27a8aec0d9ea983a1db8b079cc4412 100644 (file)
--- a/lib/private/Template/IconsCacher.php
+++ b/lib/private/Template/IconsCacher.php
@@ -238,6 +238,9 @@ class IconsCacher {
                }
        }
 
+       /**
+        * Add the icons cache css into the header
+        */
        public function injectCss() {
                $mtime = $this->timeFactory->getTime();
                $file = $this->getCachedList();

diff --git a/lib/private/Template/SCSSCacher.php b/lib/private/Template/SCSSCacher.php
index c4f89a9c63cf1a1a751b6b12ae84b17fff34ec8d..9bdaca3a674d1d9b5620044a7313d2f8a967ab89 100644 (file)
--- a/lib/private/Template/SCSSCacher.php
+++ b/lib/private/Template/SCSSCacher.php
@@ -32,6 +32,7 @@ use Leafo\ScssPhp\Compiler;
 use Leafo\ScssPhp\Exception\ParserException;
 use Leafo\ScssPhp\Formatter\Crunched;
 use Leafo\ScssPhp\Formatter\Expanded;
+use OC\Memcache\NullCache;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\IAppData;
 use OCP\Files\NotFoundException;
@@ -42,6 +43,7 @@ use OCP\ICache;
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\ILogger;
+use OCP\IMemcache;
 use OCP\IURLGenerator;
 use OC\Files\AppData\Factory;
 use OC\Template\IconsCacher;
@@ -84,6 +86,9 @@ class SCSSCacher {
        /** @var ITimeFactory */
        private $timeFactory;
 
+       /** @var IMemcache */
+       private $lockingCache;
+
        /**
         * @param ILogger $logger
         * @param Factory $appDataFactory
@@ -111,8 +116,13 @@ class SCSSCacher {
                $this->defaults     = $defaults;
                $this->serverRoot   = $serverRoot;
                $this->cacheFactory = $cacheFactory;
-               $this->depsCache    = $cacheFactory->createDistributed('SCSS-' . md5($this->urlGenerator->getBaseUrl()));
+               $this->depsCache    = $cacheFactory->createDistributed('SCSS-deps-' . md5($this->urlGenerator->getBaseUrl()));
                $this->isCachedCache = $cacheFactory->createLocal('SCSS-cached-' . md5($this->urlGenerator->getBaseUrl()));
+               $lockingCache = $cacheFactory->createDistributed('SCSS-locks-' . md5($this->urlGenerator->getBaseUrl()));
+               if (!($lockingCache instanceof IMemcache)) {
+                       $lockingCache = new NullCache();
+               }
+               $this->lockingCache = $lockingCache;
                $this->iconsCacher = $iconsCacher;
                $this->timeFactory = $timeFactory;
        }
@@ -137,10 +147,7 @@ class SCSSCacher {
 
                if (!$this->variablesChanged() && $this->isCached($fileNameCSS, $app)) {
                        // Inject icons vars css if any
-                       if ($this->iconsCacher->getCachedCSS() && $this->iconsCacher->getCachedCSS()->getSize() > 0) {
-                               $this->iconsCacher->injectCss();
-                       }
-                       return true;
+                       return $this->injectCssVariablesIfAny();
                }
 
                try {
@@ -150,7 +157,35 @@ class SCSSCacher {
                        $folder = $this->appData->newFolder($app);
                }
 
-               $cached = $this->cache($path, $fileNameCSS, $fileNameSCSS, $folder, $webDir);
+               $lockKey = $webDir . '/' . $fileNameSCSS;
+
+               if (!$this->lockingCache->add($lockKey, 'locked!', 120)) {
+                       $retry = 0;
+                       sleep(1);
+                       while ($retry < 10) {
+                               if (!$this->variablesChanged() && $this->isCached($fileNameCSS, $app)) {
+                                       // Inject icons vars css if any
+                                       $this->lockingCache->remove($lockKey);
+                                       $this->logger->debug('SCSSCacher: ' .$lockKey.' is now available after '.$retry.'s. Moving on...', ['app' => 'core']);
+                                       return $this->injectCssVariablesIfAny();
+                               }
+                               $this->logger->debug('SCSSCacher: scss cache file locked for '.$lockKey, ['app' => 'core']);
+                               sleep($retry);
+                               $retry++;
+                       }
+                       $this->logger->debug('SCSSCacher: Giving up scss caching for '.$lockKey, ['app' => 'core']);
+                       return false;
+               }
+
+               try {
+                       $cached = $this->cache($path, $fileNameCSS, $fileNameSCSS, $folder, $webDir);
+               } catch (\Exception $e) {
+                       $this->lockingCache->remove($lockKey);
+                       throw $e;
+               }
+
+               // Cleaning lock
+               $this->lockingCache->remove($lockKey);
 
                // Inject icons vars css if any
                if ($this->iconsCacher->getCachedCSS() && $this->iconsCacher->getCachedCSS()->getSize() > 0) {
@@ -180,19 +215,24 @@ class SCSSCacher {
         */
        private function isCached(string $fileNameCSS, string $app) {
                $key = $this->config->getSystemValue('version') . '/' . $app . '/' . $fileNameCSS;
-               if (!$this->config->getSystemValue('debug') && $cacheValue = $this->isCachedCache->get($key)) {
+
+               // If the file mtime is more recent than our cached one,
+               // let's consider the file is properly cached
+               if ($cacheValue = $this->isCachedCache->get($key)) {
                        if ($cacheValue > $this->timeFactory->getTime()) {
                                return true;
                        }
                }
 
+               // Creating file cache if none for further checks
                try {
                        $folder = $this->appData->getFolder($app);
                } catch (NotFoundException $e) {
-                       // creating css appdata folder
-                       $folder = $this->appData->newFolder($app);
+                       return false;
                }
 
+               // Checking if file size is coherent
+               // and if one of the css dependency changed
                try {
                        $cachedFile = $folder->getFile($fileNameCSS);
                        if ($cachedFile->getSize() > 0) {
@@ -201,7 +241,7 @@ class SCSSCacher {
                                if ($deps === null) {
                                        $depFile = $folder->getFile($depFileName);
                                        $deps    = $depFile->getContent();
-                                       //Set to memcache for next run
+                                       // Set to memcache for next run
                                        $this->depsCache->set($folder->getName() . '-' . $depFileName, $deps);
                                }
                                $deps = json_decode($deps, true);
@@ -228,13 +268,11 @@ class SCSSCacher {
         */
        private function variablesChanged(): bool {
                $injectedVariables = $this->getInjectedVariables();
-               if ($this->config->getAppValue('core', 'scss.variables') !== md5($injectedVariables)) {
+               if ($this->config->getAppValue('core', 'theming.variables') !== md5($injectedVariables)) {
                        $this->resetCache();
-                       $this->config->setAppValue('core', 'scss.variables', md5($injectedVariables));
-
+                       $this->config->setAppValue('core', 'theming.variables', md5($injectedVariables));
                        return true;
                }
-
                return false;
        }
 
@@ -289,7 +327,7 @@ class SCSSCacher {
                                '@import "functions.scss";' .
                                '@import "' . $fileNameSCSS . '";');
                } catch (ParserException $e) {
-                       $this->logger->error($e, ['app' => 'core']);
+                       $this->logger->logException($e, ['app' => 'core']);
 
                        return false;
                }
@@ -327,7 +365,11 @@ class SCSSCacher {
         */
        public function resetCache() {
                $this->injectedVariables = null;
-               $this->cacheFactory->createDistributed('SCSS-')->clear();
+
+               // do not clear locks
+               $this->cacheFactory->createDistributed('SCSS-deps-')->clear();
+               $this->cacheFactory->createDistributed('SCSS-cached-')->clear();
+
                $appDirectory = $this->appData->getDirectoryListing();
                foreach ($appDirectory as $folder) {
                        foreach ($folder->getDirectoryListing() as $file) {
@@ -338,6 +380,7 @@ class SCSSCacher {
                                }
                        }
                }
+               $this->logger->debug('SCSSCacher: css cache cleared!');
        }
 
        /**
@@ -358,7 +401,7 @@ class SCSSCacher {
                        $scss->compile($variables);
                        $this->injectedVariables = $variables;
                } catch (ParserException $e) {
-                       $this->logger->error($e, ['app' => 'core']);
+                       $this->logger->logException($e, ['app' => 'core']);
                }
 
                return $variables;
@@ -391,7 +434,7 @@ class SCSSCacher {
                return substr($this->urlGenerator->linkToRoute('core.Css.getCss', [
                        'fileName' => $fileName,
                        'appName' => $appName,
-                       'v' => $this->config->getAppValue('core', 'scss.variables', '0')
+                       'v' => $this->config->getAppValue('core', 'theming.variables', '0')
                ]), \strlen(\OC::$WEBROOT) + 1);
        }
 
@@ -449,4 +492,17 @@ class SCSSCacher {
 
                return $webRoot . substr($path, strlen($serverRoot));
        }
+
+       /**
+        * Add the icons css cache in the header if needed 
+        *
+        * @return boolean true
+        */
+       private function injectCssVariablesIfAny() {
+               // Inject icons vars css if any
+               if ($this->iconsCacher->getCachedCSS() && $this->iconsCacher->getCachedCSS()->getSize() > 0) {
+                       $this->iconsCacher->injectCss();
+               }
+               return true;
+       }
 }

diff --git a/tests/lib/Template/SCSSCacherTest.php b/tests/lib/Template/SCSSCacherTest.php
index d84b96773bcdf226a3b53d7edc4c521cec760fdd..d4d60c2dc1ba058b86d6ac54d3c5935f4e8a1f53 100644 (file)
--- a/tests/lib/Template/SCSSCacherTest.php
+++ b/tests/lib/Template/SCSSCacherTest.php
@@ -528,10 +528,10 @@ class SCSSCacherTest extends \Test\TestCase {
                        ->willReturn([$file]);
 
                $cache = $this->createMock(ICache::class);
-               $this->cacheFactory->expects($this->once())
+               $this->cacheFactory->expects($this->exactly(2))
                        ->method('createDistributed')
                        ->willReturn($cache);
-               $cache->expects($this->once())
+               $cache->expects($this->exactly(2))
                        ->method('clear')
                        ->with('');
                $this->appData->expects($this->once())