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>

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();

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);
 		}
 
+		$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) {
@@ -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;
+	}
 }

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())