Write gem files atomically #9154
Conversation
eileencodes
force-pushed
the
write-files-atomically
branch
5 times, most recently
from
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.
There was a problem hiding this comment.
Could you add a comment that this is borrowed from Active Support?
Active Support is licensed under MIT license https://github.com/rails/rails/blob/main/MIT-LICENSE . We must mention copyright and license information to use MIT licensed code.
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?
eileencodes
force-pushed
the
write-files-atomically
branch
2 times, most recently
from
f46d937 to
f84222a
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
write-files-atomically
branch
from
f84222a to
c228f4f
Compare
|
CI seems to be broken, but for JRuby only. Apparently |
4 participants
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