ENGCOM-4534: Fixes race condition when building merged css/js file du…

…ring simultaneous requests magento#21756
Hypernova-Oy · Apr 21, 2019 · 4f9791a · 4f9791a
2 parents b0ee080 + eae0bf5
commit 4f9791a
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 20 deletions.
diff --git a/lib/internal/Magento/Framework/View/Asset/MergeStrategy/Direct.php b/lib/internal/Magento/Framework/View/Asset/MergeStrategy/Direct.php
@@ -6,6 +6,8 @@
 namespace Magento\Framework\View\Asset\MergeStrategy;
 
 use Magento\Framework\App\Filesystem\DirectoryList;
+use Magento\Framework\App\ObjectManager;
+use Magento\Framework\Math\Random;
 use Magento\Framework\View\Asset;
 
 /**
@@ -30,38 +32,46 @@ class Direct implements \Magento\Framework\View\Asset\MergeStrategyInterface
      */
     private $cssUrlResolver;
 
+    /**
+     * @var Random
+     */
+    private $mathRandom;
+
     /**
      * @param \Magento\Framework\Filesystem $filesystem
      * @param \Magento\Framework\View\Url\CssResolver $cssUrlResolver
+     * @param Random|null $mathRandom
      */
     public function __construct(
         \Magento\Framework\Filesystem $filesystem,
-        \Magento\Framework\View\Url\CssResolver $cssUrlResolver
+        \Magento\Framework\View\Url\CssResolver $cssUrlResolver,
+        Random $mathRandom = null
     ) {
         $this->filesystem = $filesystem;
         $this->cssUrlResolver = $cssUrlResolver;
+        $this->mathRandom = $mathRandom ?: ObjectManager::getInstance()->get(Random::class);
     }
 
     /**
-     * {@inheritdoc}
+     * @inheritdoc
      */
     public function merge(array $assetsToMerge, Asset\LocalInterface $resultAsset)
     {
         $mergedContent = $this->composeMergedContent($assetsToMerge, $resultAsset);
         $filePath = $resultAsset->getPath();
+        $tmpFilePath = $filePath . $this->mathRandom->getUniqueHash('_');
         $staticDir = $this->filesystem->getDirectoryWrite(DirectoryList::STATIC_VIEW);
         $tmpDir = $this->filesystem->getDirectoryWrite(DirectoryList::TMP);
-        $tmpDir->writeFile($filePath, $mergedContent);
-        $tmpDir->renameFile($filePath, $filePath, $staticDir);
+        $tmpDir->writeFile($tmpFilePath, $mergedContent);
+        $tmpDir->renameFile($tmpFilePath, $filePath, $staticDir);
     }
 
     /**
      * Merge files together and modify content if needed
      *
-     * @param \Magento\Framework\View\Asset\MergeableInterface[] $assetsToMerge
-     * @param \Magento\Framework\View\Asset\LocalInterface $resultAsset
-     * @return string
-     * @throws \Magento\Framework\Exception\LocalizedException
+     * @param array $assetsToMerge
+     * @param Asset\LocalInterface $resultAsset
+     * @return array|string
      */
     private function composeMergedContent(array $assetsToMerge, Asset\LocalInterface $resultAsset)
     {

diff --git a/lib/internal/Magento/Framework/View/Test/Unit/Asset/MergeStrategy/DirectTest.php b/lib/internal/Magento/Framework/View/Test/Unit/Asset/MergeStrategy/DirectTest.php
@@ -5,13 +5,19 @@
  */
 namespace Magento\Framework\View\Test\Unit\Asset\MergeStrategy;
 
-use Magento\Framework\Filesystem\Directory\WriteInterface;
-use \Magento\Framework\View\Asset\MergeStrategy\Direct;
-
 use Magento\Framework\App\Filesystem\DirectoryList;
+use Magento\Framework\Filesystem\Directory\WriteInterface;
+use Magento\Framework\View\Asset\MergeStrategy\Direct;
 
+/**
+ * Test for Magento\Framework\View\Asset\MergeStrategy\Direct.
+ */
 class DirectTest extends \PHPUnit\Framework\TestCase
 {
+    /**
+     * @var \Magento\Framework\Math\Random|\PHPUnit_Framework_MockObject_MockObject
+     */
+    protected $mathRandomMock;
     /**
      * @var \Magento\Framework\View\Asset\MergeStrategy\Direct
      */
@@ -50,33 +56,47 @@ protected function setUp()
                 [DirectoryList::TMP, \Magento\Framework\Filesystem\DriverPool::FILE, $this->tmpDir],
             ]);
         $this->resultAsset = $this->createMock(\Magento\Framework\View\Asset\File::class);
-        $this->object = new Direct($filesystem, $this->cssUrlResolver);
+        $this->mathRandomMock = $this->getMockBuilder(\Magento\Framework\Math\Random::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $this->object = new Direct($filesystem, $this->cssUrlResolver, $this->mathRandomMock);
     }
 
     public function testMergeNoAssets()
     {
+        $uniqId = '_b3bf82fa6e140594420fa90982a8e877';
         $this->resultAsset->expects($this->once())->method('getPath')->will($this->returnValue('foo/result'));
         $this->staticDir->expects($this->never())->method('writeFile');
-        $this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result', '');
-        $this->tmpDir->expects($this->once())->method('renameFile')->with('foo/result', 'foo/result', $this->staticDir);
+        $this->mathRandomMock->expects($this->once())
+            ->method('getUniqueHash')
+            ->willReturn($uniqId);
+        $this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result' . $uniqId, '');
+        $this->tmpDir->expects($this->once())->method('renameFile')
+            ->with('foo/result' . $uniqId, 'foo/result', $this->staticDir);
         $this->object->merge([], $this->resultAsset);
     }
 
     public function testMergeGeneric()
     {
+        $uniqId = '_be50ccf992fd81818c1a2645d1a29e92';
         $this->resultAsset->expects($this->once())->method('getPath')->will($this->returnValue('foo/result'));
         $assets = $this->prepareAssetsToMerge([' one', 'two']); // note leading space intentionally
         $this->staticDir->expects($this->never())->method('writeFile');
-        $this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result', 'onetwo');
-        $this->tmpDir->expects($this->once())->method('renameFile')->with('foo/result', 'foo/result', $this->staticDir);
+        $this->mathRandomMock->expects($this->once())
+            ->method('getUniqueHash')
+            ->willReturn($uniqId);
+        $this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result' . $uniqId, 'onetwo');
+        $this->tmpDir->expects($this->once())->method('renameFile')
+            ->with('foo/result' . $uniqId, 'foo/result', $this->staticDir);
         $this->object->merge($assets, $this->resultAsset);
     }
 
     public function testMergeCss()
     {
+        $uniqId = '_f929c374767e00712449660ea673f2f5';
         $this->resultAsset->expects($this->exactly(3))
             ->method('getPath')
-            ->will($this->returnValue('foo/result'));
+            ->willReturn('foo/result');
         $this->resultAsset->expects($this->any())->method('getContentType')->will($this->returnValue('css'));
         $assets = $this->prepareAssetsToMerge(['one', 'two']);
         $this->cssUrlResolver->expects($this->exactly(2))
@@ -85,10 +105,14 @@ public function testMergeCss()
         $this->cssUrlResolver->expects($this->once())
             ->method('aggregateImportDirectives')
             ->with('12')
-            ->will($this->returnValue('1020'));
+            ->willReturn('1020');
+        $this->mathRandomMock->expects($this->once())
+            ->method('getUniqueHash')
+            ->willReturn($uniqId);
         $this->staticDir->expects($this->never())->method('writeFile');
-        $this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result', '1020');
-        $this->tmpDir->expects($this->once())->method('renameFile')->with('foo/result', 'foo/result', $this->staticDir);
+        $this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result' . $uniqId, '1020');
+        $this->tmpDir->expects($this->once())->method('renameFile')
+            ->with('foo/result' . $uniqId, 'foo/result', $this->staticDir);
         $this->object->merge($assets, $this->resultAsset);
     }