Fixes race condition when building merged css/js file during simultaneous requests #21756
Conversation
…request to build the same merged css or js file sometimes results in serving incomplete files to the client if a second request came in and truncated the part of the work from the first request.
|
Hi @Ian410. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
magento-engcom-team
added
Component: View
Release Line: 2.3
partners-contribution
| /** | ||
| * @var \Magento\Framework\Math\Random | ||
| */ | ||
| protected $mathRandom; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Resolved in latest commit.
| $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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Request A creates file var/tmp/_cache/merged/abc123abc123.min.css_ZpAA9ljK7QDGf5az2pTLEv55adF7
- Request B creates file var/tmp/_cache/merged/abc123abc123.min.css_cOgz75FD4tcK0ALxDpCM
- Request A finishes creation of file and renames var/tmp/_cache/merged/abc123abc123.min.css_ZpAA9ljK7QDGf5az2pTLEv55adF7 to pub/static/_cache/merged/abc123abc123.min
- 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.
There was a problem hiding this comment.
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.
…l guidelines Made mathrandom class optional to meet backwards compatibility requirements.
|
@Ian410 unfortunately, only members of the maintainers team are allowed to remove progress related labels to the pull request |
|
@Ian410 thanks for changes |
|
Hi @VladimirZaets, thank you for the review. |
ghost
added
Progress: testing in progress
Progress: extended testing
and removed
Progress: ready for testing
Progress: testing in progress
labels
|
QA passed. |
p-bystritsky
force-pushed
the
feature/merged-static-content-race-condition
branch
from
d6aaa2b
to
4d37719
Compare
|
Hi @Ian410, thank you for your contribution! |
…ring simultaneous requests magento#21756
Description (*)
Fixes bug when many requests to a page are made and multiple streams request to build the same merged css or js file sometimes results in serving incomplete files to the client if a second request came in and truncated the part of the work from the first request.
If two requests try and build a merged css or js file both attempt to do so. Request A starts to build the file, when Request B starts it truncates the temporary file and begins to build its own. Request A finishes the process and copied a now incomplete file into the final directory and serves it to the customer. When Request B finishes, it copes the now complete file into the final directory and serves it to the customer. Only the first customer would notice a problem and it would be resolved on refresh.
This can manifest itself more severely with a CDN. In the scenario above, the incomplete file would be cached by a CDN edge node. If request A came from Chicago and request B came from Miami then Chicago node would have the incomplete file and Miami would have the complete file. All users from Chicago and surrounding areas would have the site broken while it would appear fine to people in Miami.
Manual testing scenarios (*)
Contribution checklist (*)