https://git.ruby-lang.org/ruby.git/commit/?id=7fd88da935

From 7fd88da935c7c6fcafe19cf30642676033ec82bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <deivid.rodriguez@r...>
Date: Thu, 25 Feb 2021 18:43:51 +0100
Subject: [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
---
 lib/rubygems.rb                     |  8 ++++----
 lib/rubygems/installer.rb           | 12 +++---------
 test/rubygems/test_gem_installer.rb | 27 +++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/lib/rubygems.rb b/lib/rubygems.rb
index 80708e2aa00..37984157de7 100644
--- a/lib/rubygems.rb
+++ b/lib/rubygems.rb
@@ -800,11 +800,11 @@ An Array (#{env.inspect}) was passed in from #{caller[3]} https://github.com/ruby/ruby/blob/trunk/lib/rubygems.rb#L800
   ##
   # 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
diff --git a/lib/rubygems/installer.rb b/lib/rubygems/installer.rb
index 38642ee8ef4..8e3965ef92d 100644
--- a/lib/rubygems/installer.rb
+++ b/lib/rubygems/installer.rb
@@ -446,13 +446,9 @@ class Gem::Installer https://github.com/ruby/ruby/blob/trunk/lib/rubygems/installer.rb#L446
   # 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 https://github.com/ruby/ruby/blob/trunk/lib/rubygems/installer.rb#L456
   # 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
 
   ##
diff --git a/test/rubygems/test_gem_installer.rb b/test/rubygems/test_gem_installer.rb
index 8874577aa8a..dae2b070d57 100644
--- a/test/rubygems/test_gem_installer.rb
+++ b/test/rubygems/test_gem_installer.rb
@@ -288,6 +288,33 @@ gem 'other', version https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_installer.rb#L288
                  "(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
 
-- 
cgit v1.2.1


--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/