Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes race condition when building merged css/js file during simultaneous requests #21756

Merged
merged 3 commits into from
Apr 23, 2019
Merged

Fixes race condition when building merged css/js file during simultaneous requests #21756

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b46c96
Fixes bug when many requests to a page are made and multiple streams …
Ian410 Mar 14, 2019
c6affac
Updated mathrandom class property to private to meet Magento technica…
Ian410 Mar 15, 2019
4d37719
magento/magento2#21756: Static test fix.
p-bystritsky Apr 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions lib/internal/Magento/Framework/View/Asset/MergeStrategy/Direct.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,23 @@ class Direct implements \Magento\Framework\View\Asset\MergeStrategyInterface
*/
private $cssUrlResolver;

/**
* @var \Magento\Framework\Math\Random
*/
protected $mathRandom;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Ian410. Thanks for collaboration. According to [Magento Technical Guidelines] we can provide new public/protected properties (https://devdocs.magento.com/guides/v2.3/coding-standards/technical-guidelines.html)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in latest commit.


/**
* @param \Magento\Framework\Filesystem $filesystem
* @param \Magento\Framework\View\Url\CssResolver $cssUrlResolver
*/
public function __construct(
\Magento\Framework\Filesystem $filesystem,
\Magento\Framework\View\Url\CssResolver $cssUrlResolver
\Magento\Framework\View\Url\CssResolver $cssUrlResolver,
\Magento\Framework\Math\Random $mathRandom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to Magento backward-compatible guide we can't add required parameters to the constructor. Please look Adding a constructor parameter section to do it in the correct way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in latest commit.

) {
$this->filesystem = $filesystem;
$this->cssUrlResolver = $cssUrlResolver;
$this->mathRandom = $mathRandom;
}

/**
Expand All @@ -49,17 +56,18 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the file always will have a different name. I'm sure that resolves the problem, but the file will not caching and always will regenerate. If I am right that is not pretty cool.

Copy link
Member Author

@Ian410 Ian410 Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct that each temporary file will have a different filename. However, the next line not commented is $tmpDir->renameFile($tmpFilePath, $filePath, $staticDir); which always renames the file to the correct path.

For example, in my two request example:

  1. Request A creates file var/tmp/_cache/merged/abc123abc123.min.css_ZpAA9ljK7QDGf5az2pTLEv55adF7
  2. Request B creates file var/tmp/_cache/merged/abc123abc123.min.css_cOgz75FD4tcK0ALxDpCM
  3. Request A finishes creation of file and renames var/tmp/_cache/merged/abc123abc123.min.css_ZpAA9ljK7QDGf5az2pTLEv55adF7 to pub/static/_cache/merged/abc123abc123.min
  4. Request B finishes creation of file and renames var/tmp/_cache/merged/abc123abc123.min.css_cOgz75FD4tcK0ALxDpCM to pub/static/_cache/merged/abc123abc123.min

Request B does override the Request A file that was built, but it is the same file so it doesn't matter. The important part is that Request A didn't copy over a truncated file as Request A was building it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That last sentence should read:
The important part is that Request A didn't copy over a truncated file as Request B started building it.

$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
* @param \Magento\ Framework\View\Asset\LocalInterface $resultAsset
* @return string
* @throws \Magento\Framework\Exception\LocalizedException
*/
Expand Down
33 changes: 26 additions & 7 deletions lib/internal/Magento/Framework/View/Test/Unit/Asset/MergeStrategy/DirectTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@

class DirectTest extends \PHPUnit\Framework\TestCase
{
/**
* @var \Magento\Framework\Math\Random|\PHPUnit_Framework_MockObject_MockObject
*/
protected $mathRandomMock;
/**
* @var \Magento\Framework\View\Asset\MergeStrategy\Direct
*/
Expand Down Expand Up @@ -50,30 +54,42 @@ 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'));
Expand All @@ -86,9 +102,12 @@ public function testMergeCss()
->method('aggregateImportDirectives')
->with('12')
->will($this->returnValue('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);
}

Expand Down