Merge pull request #2029 from brigade/lock-example-status

Lock example_status_persistence_file to prevent race conditions

Author: myronmarston

lib/rspec/core/example_status_persister.rb

@@ -21,16 +21,22 @@ def initialize(examples, file_name)
       end
 
       def persist
-        write dumped_statuses
+        RSpec::Support::DirectoryMaker.mkdir_p(File.dirname(@file_name))
+        File.open(@file_name, File::RDWR | File::CREAT) do |f|
+          # lock the file while reading / persisting to avoid a race
+          # condition where parallel or unrelated spec runs race to
+          # update the same file
+          f.flock(File::LOCK_EX)
+          @previous_runs = f.read
+          f.rewind
+          f.write(dumped_statuses)
+          f.flush
+          f.truncate(f.pos)
+        end
       end
 
     private
 
-      def write(statuses)
-        RSpec::Support::DirectoryMaker.mkdir_p(File.dirname(@file_name))
-        File.open(@file_name, "w") { |f| f.write(statuses) }
-      end
-
       def dumped_statuses
         ExampleStatusDumper.dump(merged_statuses)
       end
@@ -52,7 +58,7 @@ def statuses_from_this_run
       end
 
       def statuses_from_previous_runs
-        self.class.load_from(@file_name)
+        ExampleStatusParser.parse(@previous_runs)
       end
     end
 

spec/rspec/core/example_status_persister_spec.rb

@@ -58,6 +58,36 @@ def new_example(id, metadata = {})
         )
       end
 
+      it 'prevents simultaneous access to the file' do
+        # This tests whether a certain race condition is prevented:
+        #  - read 1
+        #  - read 2
+        #  - write 1
+        #  - write 2 - write 1 is lost
+        ex_1 = new_example("#{existing_spec_file}[1:1]", :status => :passed)
+        ex_2 = new_example("spec_1.rb[1:1]", :status => :failed)
+
+        persister_1 = ExampleStatusPersister.new([ex_1], file.path)
+        persister_2 = ExampleStatusPersister.new([ex_2], file.path)
+        persister_2_thread = nil
+
+        # dumped_statuses is called after the file is locked but
+        # before the output is written
+        allow(persister_1).to receive(:dumped_statuses).and_wrap_original do |m, *args|
+          persister_2_thread = Thread.new { persister_2.persist }
+          m.call(*args)
+        end
+        persister_1.persist
+        persister_2_thread.join
+
+        loaded = ExampleStatusPersister.load_from(file.path)
+
+        expect(loaded).to contain_exactly(
+          a_hash_including(:example_id => ex_1.id, :status => "passed"),
+          a_hash_including(:example_id => ex_2.id, :status => "failed")
+        )
+      end
+
       it 'merges the example statuses with the existing records in the named file' do
         ex_1 = new_example("#{existing_spec_file}[1:1]", :status => :passed)
         ex_2 = new_example("spec_1.rb[1:1]", :status => :failed)