Write gem files atomically #9154
Conversation
c2611a4 to
79df6f8
Compare
|
Working on the test failure. |
| def fake_fetcher(url, data) | ||
| fetcher = Gem::FakeFetcher.new | ||
| fetcher.data[url] = data | ||
| Gem::RemoteFetcher.fetcher = fetcher |
There was a problem hiding this comment.
This mutates global state. Do we need to save the previous value and set it back after calling this method?
There was a problem hiding this comment.
I think technically it's fine because setup always sets fetcher to nil. See
but I'll add an ensure anyway in case something gets changed in the future.
lib/rubygems/util/atomic_file.rb
Outdated
| class AtomicFile | ||
| ## | ||
| # Write to a file atomically. Useful for situations where you don't | ||
| # want other processes or threads to see half-written files. | ||
|
|
||
| def self.write(file_name) |
There was a problem hiding this comment.
How about renaming to AtomicFileWriter.open or something because this method itself doesn't write a file content? (Block write a content)
lib/rubygems/util/atomic_file.rb
Outdated
|
|
||
| Tempfile.open(".#{File.basename(file_name)}", temp_dir) do |temp_file| | ||
| temp_file.binmode | ||
| return_file = yield temp_file |
There was a problem hiding this comment.
Can we rename return_file to return_value or something because it may not be a file?
lib/rubygems/util/atomic_file.rb
Outdated
| FileUtils.touch(file_name) | ||
| File.stat(file_name) | ||
| rescue Errno::ENOENT | ||
| file_name = nil | ||
| ensure | ||
| FileUtils.rm_f(file_name) if file_name |
There was a problem hiding this comment.
Can we remove fileutils dependency by using File.open(file_name, "w") {} and begin; File.unlink(file_name); rescue SystemCallError; end?
f84222a to
c228f4f
Compare
|
CI seems to be broken, but for JRuby only. Apparently |
c228f4f to
4e2c8b3
Compare
4e2c8b3 to
154b78b
Compare
|
I addressed the feedback and fixed the failing tests. Let me know if there are other changes you'd like me to make. |
|
|
||
| # Based on ActiveSupport's AtomicFile implementation | ||
| # Copyright (c) David Heinemeier Hansson | ||
| # https://github.com/rails/rails/blob/main/activesupport/lib/active_support/atomic_file.rb |
There was a problem hiding this comment.
| # https://github.com/rails/rails/blob/main/activesupport/lib/active_support/atomic_file.rb | |
| # https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/file/atomic.rb |
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.
154b78b to
0cd4b54
Compare
This change updates
write_binaryto use a new class,AtomicFile.writeto write the gem's files. This implementation is borrowed from Active Support'satomic_write.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_gzobsolete, 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
RemoteFetcherstub withFakeFetcherexcept for where we really do need to overwrite theRemoteFetcher. The new test implementation is much clearer on what it's trying to accomplish versus the prior test implementation.Make sure the following tasks are checked