Refactor atomic file write #9202
Conversation
eileencodes
force-pushed
the
refactor-atomic-write
branch
from
4927f00 to
1e09dbb
Compare
This change updates `write_binary` to use a new class, `AtomicFileWriter.open` to write the gem's files. This implementation is borrowed from Active Support's [`atomic_write`](https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/file/atomic.rb). Atomic write will write the files to a temporary file and then once created, sets permissions and renames the file. If the file is corrupted - ie on failed download, an error occurs, or for some other reason, the real file will not be created. The changes made here make `verify_gz` obsolete, we don't need to verify it if we have successfully created the file atomically. If it exists, it is not corrupt. If it is corrupt, the file won't exist on disk. While writing tests for this functionality I replaced the `RemoteFetcher` stub with `FakeFetcher` except for where we really do need to overwrite the `RemoteFetcher`. The new test implementation is much clearer on what it's trying to accomplish versus the prior test implementation.
eileencodes
force-pushed
the
refactor-atomic-write
branch
2 times, most recently
from
eaebe26 to
dfbbae6
Compare
| # Set correct permissions on new file | ||
| if original_permissions | ||
| File.open(tmp_path, flags) do |temp_file| | ||
| temp_file.binmode |
There was a problem hiding this comment.
Do we need this? We already specified File::BINARY.
There was a problem hiding this comment.
I think it likely is redundant, I copied it from the changes in Rails. I could see it being useful as extra safety, but it's probably fine to remove. Would you rather I remove it?
There was a problem hiding this comment.
Let's remove redundant code for easy to maintain!
| rescue SystemCallError | ||
| raise error | ||
| else | ||
| File.rename(temp_file.path, file_name) |
There was a problem hiding this comment.
Do we need to care about File.rename failure?
| File.rename(temp_file.path, file_name) | |
| begin | |
| File.rename(temp_file.path, file_name) | |
| rescue StandardError | |
| begin | |
| File.unlink(temp_file.path) | |
| rescue StandardError | |
| end | |
| raise | |
| end |
There was a problem hiding this comment.
Sorry, do you mean "we should also rescue File.rename"? or something else? I think these changes are easier to look at split or just the new file rather than the diff.
There was a problem hiding this comment.
Sorry for my confusing comment.
"we should also rescue File.rename"
This.
I think that if File.rename is failed, the temp_file.path exists.
This refactoring is based off the changes in test/rubygems/test_gem_remote_fetcher.rb. It no longer uses tempfile as a result.
eileencodes
force-pushed
the
refactor-atomic-write
branch
from
dfbbae6 to
3099391
Compare
3 participants
This refactoring is based off the changes in
test/rubygems/test_gem_remote_fetcher.rb. It no longer uses tempfile as a result.
Make sure the following tasks are checked