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.

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

Fix writeFile() mkdir() race condition #379

Conversation

fixpunkt
Copy link

PHP's mkdir() is not concurrency-safe when the $recursive flag is enabled. When multiple concurrent processes try to create paths within the same directory structure, interleaving execution of mkdir() may cause some of those processes to fail if another process creates a subdirectory they were just about to create. Please see https://bugs.php.net/bug.php?id=35326 for detailed discussion.

This race condition may cause Smarty template compilation to fail randomly when not all templates have been initialized. Lately, we've observed this problem with increasing frequency in https://github.com/shopware/shopware.

@a-shpota has added code that detects this situation and throws a more meaningful message in 6768340. Unfortunately, template compilation will still fail even with these changes applied.

This PR adds a concurrency-safe PHP reimplementation of recursive mkdir(). Using this implementation, writeFile() will not fail anymore in situations that would have triggered the mkdir() race condition before.

@fixpunkt fixpunkt force-pushed the viison/fix-cache-mkdir-race-condition branch from f960d31 to 1ae6b5d Compare July 25, 2017 08:43
fixpunkt added a commit to pickware/shopware that referenced this pull request Jul 25, 2017
This is a backport of the code in
smarty-php/smarty#379 to Shopware's vendored
Smarty 3.1.8.
@uwetews
Copy link
Contributor

uwetews commented Jul 30, 2017

This pull request is rejected as it calls mkdir() on each file write for each subfolder if needed or not.

Meanwhile libs/sysplugins/smarty_internal_runtime_writefile.php has been rewritten to retry in case of error.

The fix is in the trunk version and will later be included in 3.1.32

Note:
I recommend also to set $smarty->cache_locking = true;
To prevent on heavily loaded servers possible concurrent cache file updates of same page.

@uwetews uwetews closed this Jul 30, 2017
@fixpunkt
Copy link
Author

Good point about the cache locking, I was unaware of this!

@fixpunkt
Copy link
Author

fixpunkt commented Jul 31, 2017

I've just tried both of your approaches on a system where I was able to reproduce the issue consistently.

Enabling $smarty->cache_locking had no effect on the issue. I verified that it was set correctly by adding the value of the property to the throws clause.

I've also tried your workaround for the race condition. The effect of this was that I was getting unable to create directory ... Exceptions all the time now. I also tried increasing the maximum number of retries to 10, which caused the application to no longer load.

uwetews added a commit that referenced this pull request Jul 31, 2017
@uwetews
Copy link
Contributor

uwetews commented Jul 31, 2017

I have added a clearstatcache() just in case.
Are you sure that compile_dir and cache_dir had right permissions?

@fixpunkt
Copy link
Author

fixpunkt commented Aug 1, 2017

Yes, I'm sure that the permissions are correct. After all, everything works when I use the fix I've provided in this PR.

I've also tested the version of your fix with clearstatcache(). This made the error disappear as well. Weirdly enough, I was unable to reproduce the problem by reverting to the pre-fix version after this without removing the entire cache directory. So the stat cache seems to be involved in the original issue as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants