[rubygems/rubygems] Fix race condition when reading & writing gemspecs concurrently

When bundler parallel installer installs gems concurrently, one can get
confusing warnings like the following:

```
"[/home/runner/work/rubygems/rubygems/bundler/tmp/2/gems/system/specifications/zeitwerk-2.4.2.gemspec] isn't a Gem::Specification (NilClass instead).
```

I've got these warnings several times in the past, but I never managed
to reproduce them, and never look deeply into the root cause, but this
time a got a cause that reproduced quite frequently, so I looked into
it.

The problem is one thread reading a gemspec while another thread is
writing it. The write of the gemspec was not protected, so
`Gem::Specification.load` could end up seeing a truncated gemspec and
thus throw this warning.

The fix involve two changes:

* Change the methods that write gemspecs to use `Gem.binary_write` which
  is protected by a lock.

* Fix `Gem.binary_write` to create the file lock at file creation time,
  not when the file already exists after.

The realworld user problem caused by this issue happens in bundler, but
I'm fixing it in rubygems first, and then I'll backport to bundler
whatever needs backporting to fix the issue on the bundler side.

https://github.com/rubygems/rubygems/commit/a672e7555c
This commit is contained in:
David Rodríguez 2021-02-25 18:43:51 +01:00 коммит произвёл git
Родитель d7f6cb0f78
Коммит 7fd88da935
3 изменённых файлов: 34 добавлений и 13 удалений

Просмотреть файл

@ -800,11 +800,11 @@ An Array (#{env.inspect}) was passed in from #{caller[3]}
##
# Safely write a file in binary mode on all platforms.
def self.write_binary(path, data)
File.open(path, File::RDWR | File::CREAT | File::BINARY | File::LOCK_EX) do |io|
io.write data
end
rescue *WRITE_BINARY_ERRORS
File.open(path, 'wb') do |io|
begin
io.flock(File::LOCK_EX)
rescue *WRITE_BINARY_ERRORS
end
io.write data
end
rescue Errno::ENOLCK # NFS

Просмотреть файл

@ -446,13 +446,9 @@ class Gem::Installer
# specifications directory.
def write_spec
File.open spec_file, 'w' do |file|
spec.installed_by_version = Gem.rubygems_version
spec.installed_by_version = Gem.rubygems_version
file.puts spec.to_ruby_for_cache
file.fsync rescue nil # for filesystems without fsync(2)
end
Gem.write_binary(spec_file, spec.to_ruby_for_cache)
end
##
@ -460,9 +456,7 @@ class Gem::Installer
# specifications/default directory.
def write_default_spec
File.open(default_spec_file, "w") do |file|
file.puts spec.to_ruby
end
Gem.write_binary(default_spec_file, spec.to_ruby)
end
##

Просмотреть файл

@ -288,6 +288,33 @@ gem 'other', version
"(SyntaxError)", e.message
end
def test_ensure_no_race_conditions_between_installing_and_loading_gemspecs
a, a_gem = util_gem 'a', 2
Gem::Installer.at(a_gem).install
t1 = Thread.new do
5.times do
Gem::Installer.at(a_gem).install
sleep 0.1
end
end
t2 = Thread.new do
_, err = capture_output do
20.times do
Gem::Specification.load(a.spec_file)
Gem::Specification.send(:clear_load_cache)
end
end
assert_empty err
end
t1.join
t2.join
end
def test_ensure_loadable_spec_security_policy
pend 'openssl is missing' unless Gem::HAVE_OPENSSL