[Caching] Use FileSystem::writeAtomic() in FileCacheStorage to avoid partial cache reads on parallel run - #8520
Merged
Merged
Conversation
…partial cache reads on parallel run
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative to #8426, following the suggestion to drop the hand-rolled workarounds and reuse
Nette\Utils\FileSystem::writeAtomic()(added in nette/utils 4.1.5, cc0326d).Problem
On parallel runs,
save()usedcopy()to publish the cache file.copy()truncates the destination and streams into it, so it is not atomic - a concurrent worker thatrequire()s the same path during boot can read a half-written file and crash:This hits Linux too, not only Windows.
Change
save()now callsFileSystem::writeAtomic(), which writes to a temp file andrename()s it into place - an atomic replace, so a reader always sees either the old or the new complete file. It also carries the Windows retry loop for therename()edge case that motivated the originalcopy()switch (Could not process a file due to System error: Cannot rename rector#8432, rename fails on Windows PHP 8.1 if the target file is being executed php/php-src#7910), so no separate workaround is needed.nette/utilsto^4.1.5(first release withwriteAtomic()).save()must still see the complete contents it opened, and fails the momentcopy()is put back.Fixes rectorphp/rector#9876