Publish cache files with an atomic rename - #83
Open
navidemad wants to merge 1 commit into
Open
Conversation
`FileCache#write` truncated the destination and filled it back in, so any process reading that file during the write got a prefix of the JSON and a `JSON::ParserError` -- a corrupt-cache failure with no corrupt cache behind it. Nothing in a single-process run reads a file while it is written, but a cache directory is shared as soon as the cache outlives the process that wrote it: a warm-up process that fills the cache before the suite starts, a second suite started against a preserved cache, or workers pointed at one `cache_path` (see issue Gusto#80). Writing a sibling file and renaming it over the destination publishes the new content in one step: a reader sees either the whole previous file or the whole new one. The spec forks a reader that parses the file in a loop while the parent rewrites it a hundred times; it reports a torn read through its exit status. It fails on the previous implementation and passes on this one.
Collaborator
|
Want to just use https://api.rubyonrails.org/classes/File.html#method-c-atomic_write since we depend on active support? |
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
2 participants
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.
Problem
FileCache#writewas a plainFile.write, which truncates the destination and fills it back in. A process reading that file inside the window gets a prefix of the JSON and aJSON::ParserError, which reads as a corrupt cache when nothing is actually corrupt.Nothing in a single-process run reads a file while it is being written. But the cache directory is shared as soon as the cache outlives the process that wrote it, which is exactly the setup #80 is about: a warm-up process that fills the cache before the suite starts, a second suite started against a preserved cache, or workers pointed at one
cache_path.Fix
Write a sibling temp file and rename it over the destination. The rename publishes the new content in one step, so a concurrent reader sees either the whole previous file or the whole new one. The temp file is a sibling so the rename stays on one filesystem, and carries the pid so two processes writing the same fixture cannot collide on it.
Test
The spec forks a reader that parses the file in a loop while the parent rewrites it a hundred times, and reports a torn read through its exit status. It fails on the previous implementation (checked, three runs out of three) and passes on this one (five runs out of five). It skips where
forkis unavailable and finishes in about a tenth of a second.A second spec asserts the directory holds nothing but the cache file, so a failed write cannot leave a
.tmpsibling behind.Relation to #62
#62 turns a torn read into a
CacheCorruptErrorwith a path and a hint. This removes one of the causes. They are independent and stack in either order.Full suite green: 211 examples, 0 failures.