Merge pull request #570 from jenseng/fix-result-race-condition

Fix merging race conditions

lib/simplecov.rb

@@ -75,23 +75,21 @@ def add_not_loaded_files(result)
     # from cache using SimpleCov::ResultMerger if use_merging is activated (default)
     #
     def result
-      # Ensure the variable is defined to avoid ruby warnings
-      @result = nil unless defined?(@result)
+      return @result if result?
 
       # Collect our coverage result
-      if running && !result?
+      if running
         @result = SimpleCov::Result.new add_not_loaded_files(Coverage.result)
       end
 
       # If we're using merging of results, store the current result
-      # first, then merge the results and return those
+      # first (if there is one), then merge the results and return those
       if use_merging
         SimpleCov::ResultMerger.store_result(@result) if result?
-
-        SimpleCov::ResultMerger.merged_result
-      else
-        @result
+        @result = SimpleCov::ResultMerger.merged_result
       end
+
+      @result
     ensure
       self.running = false
     end
@@ -158,6 +156,13 @@ def usable?
         false
       end
     end
+
+    #
+    # Clear out the previously cached .result. Primarily useful in testing
+    #
+    def clear_result
+      @result = nil
+    end
   end
 end
 

lib/simplecov/result_merger.rb

@@ -17,25 +17,31 @@ def resultset_writelock
         File.join(SimpleCov.coverage_path, ".resultset.json.lock")
       end
 
-      # Loads the cached resultset from JSON and returns it as a Hash
+      # Loads the cached resultset from JSON and returns it as a Hash,
+      # caching it for subsequent accesses.
       def resultset
-        if stored_data
-          begin
-            JSON.parse(stored_data)
-          rescue
+        @resultset ||= begin
+          data = stored_data
+          if data
+            begin
+              JSON.parse(data) || {}
+            rescue
+              {}
+            end
+          else
             {}
           end
-        else
-          {}
         end
       end
 
       # Returns the contents of the resultset cache as a string or if the file is missing or empty nil
       def stored_data
-        return unless File.exist?(resultset_path)
-        data = File.read(resultset_path)
-        return if data.nil? || data.length < 2
-        data
+        synchronize_resultset do
+          return unless File.exist?(resultset_path)
+          data = File.read(resultset_path)
+          return if data.nil? || data.length < 2
+          data
+        end
       end
 
       # Gets the resultset hash and re-creates all included instances
@@ -76,8 +82,9 @@ def merged_result
 
       # Saves the given SimpleCov::Result in the resultset cache
       def store_result(result)
-        File.open(resultset_writelock, "w+") do |f|
-          f.flock(File::LOCK_EX)
+        synchronize_resultset do
+          # Ensure we have the latest, in case it was already cached
+          clear_resultset
           new_set = resultset
           command_name, data = result.to_hash.first
           new_set[command_name] = data
@@ -87,6 +94,28 @@ def store_result(result)
         end
         true
       end
+
+      # Ensure only one process is reading or writing the resultset at any
+      # given time
+      def synchronize_resultset
+        # make it reentrant
+        return yield if defined?(@resultset_locked) && @resultset_locked
+
+        begin
+          @resultset_locked = true
+          File.open(resultset_writelock, "w+") do |f|
+            f.flock(File::LOCK_EX)
+            yield
+          end
+        ensure
+          @resultset_locked = false
+        end
+      end
+
+      # Clear out the previously cached .resultset
+      def clear_resultset
+        @resultset = nil
+      end
     end
   end
 end

spec/result_merger_spec.rb

@@ -1,10 +1,16 @@
 require "helper"
+require "tempfile"
+require "timeout"
 
 if SimpleCov.usable?
   describe SimpleCov::ResultMerger do
+    before do
+      SimpleCov::ResultMerger.clear_resultset
+      File.delete(SimpleCov::ResultMerger.resultset_path) if File.exist?(SimpleCov::ResultMerger.resultset_path)
+    end
+
     describe "with two faked coverage resultsets" do
       before do
-        SimpleCov.use_merging true
         @resultset1 = {
           source_fixture("sample.rb") => [nil, 1, 1, 1, nil, nil, 1, 1, nil, nil],
           source_fixture("app/models/user.rb") => [nil, 1, 1, 1, nil, nil, 1, 0, nil, nil],
@@ -73,15 +79,92 @@
               expect(SimpleCov::ResultMerger.results.length).to eq(1)
             end
           end
+        end
+      end
+    end
+
+    describe ".store_result" do
+      it "refreshes the resultset" do
+        set = SimpleCov::ResultMerger.resultset
+        SimpleCov::ResultMerger.store_result({})
+        new_set = SimpleCov::ResultMerger.resultset
+        expect(new_set).not_to be(set)
+      end
 
-          context "with merging disabled" do
-            before { SimpleCov.use_merging false }
+      it "persists to disk" do
+        SimpleCov::ResultMerger.store_result("a" => [1])
+        SimpleCov::ResultMerger.clear_resultset
+        new_set = SimpleCov::ResultMerger.resultset
+        expect(new_set).to eq("a" => [1])
+      end
+
+      it "synchronizes writes" do
+        expect(SimpleCov::ResultMerger).to receive(:synchronize_resultset)
+        SimpleCov::ResultMerger.store_result({})
+      end
+    end
 
-            it "returns nil for SimpleCov.result" do
-              expect(SimpleCov.result).to be_nil
+    describe ".resultset" do
+      it "caches" do
+        set = SimpleCov::ResultMerger.resultset
+        new_set = SimpleCov::ResultMerger.resultset
+        expect(new_set).to be(set)
+      end
+
+      it "synchronizes reads" do
+        expect(SimpleCov::ResultMerger).to receive(:synchronize_resultset)
+        SimpleCov::ResultMerger.resultset
+      end
+    end
+
+    describe ".synchronize_resultset" do
+      it "is reentrant (i.e. doesn't block its own process)" do
+        # without @resultset_locked, this spec would fail and
+        # `.store_result` wouldn't work
+        expect do
+          Timeout.timeout(1) do
+            SimpleCov::ResultMerger.synchronize_resultset do
+              SimpleCov::ResultMerger.synchronize_resultset do
+              end
             end
           end
+        end.not_to raise_error
+      end
+
+      it "blocks other processes" do
+        file = Tempfile.new("foo")
+
+        other_process = open("|ruby -e " + Shellwords.escape(<<-CODE) + " 2>/dev/null")
+          require "simplecov"
+          SimpleCov.coverage_dir(#{SimpleCov.coverage_dir.inspect})
+
+          # ensure the parent process has enough time to get a
+          # lock before we do
+          sleep 0.5
+
+          $stdout.sync = true
+          puts "running" # see `sleep`s in parent process
+
+          SimpleCov::ResultMerger.synchronize_resultset do
+            File.open(#{file.path.inspect}, "a") { |f| f.write("process 2\n") }
+          end
+        CODE
+
+        SimpleCov::ResultMerger.synchronize_resultset do
+          # wait until the child process is going, and then wait some more
+          # so we can be sure it blocks on the lock we already have.
+          sleep 0.1 until other_process.gets == "running\n"
+          sleep 1
+
+          # despite the sleeps, this will be written first since we got
+          # the first lock
+          File.open(file.path, "a") { |f| f.write("process 1\n") }
         end
+
+        # wait for it to finish
+        other_process.gets
+
+        expect(file.read).to eq("process 1\nprocess 2\n")
       end
     end
   end

spec/simplecov_spec.rb

@@ -0,0 +1,109 @@
+require "helper"
+
+if SimpleCov.usable?
+  describe SimpleCov do
+    describe ".result" do
+      before do
+        SimpleCov.clear_result
+        allow(Coverage).to receive(:result).once.and_return({})
+      end
+
+      context "with merging disabled" do
+        before do
+          allow(SimpleCov).to receive(:use_merging).once.and_return(false)
+        end
+
+        context "when not running" do
+          before do
+            allow(SimpleCov).to receive(:running).and_return(false)
+          end
+
+          it "returns nil" do
+            expect(SimpleCov.result).to be_nil
+          end
+        end
+
+        context "when running" do
+          before do
+            allow(SimpleCov).to receive(:running).and_return(true, false)
+          end
+
+          it "uses the result from Coverage" do
+            expect(Coverage).to receive(:result).once.and_return(__FILE__ => [0, 1])
+            expect(SimpleCov.result.filenames).to eq [__FILE__]
+          end
+
+          it "adds not-loaded-files" do
+            expect(SimpleCov).to receive(:add_not_loaded_files).once.and_return({})
+            SimpleCov.result
+          end
+
+          it "doesn't store the current coverage" do
+            expect(SimpleCov::ResultMerger).not_to receive(:store_result)
+            SimpleCov.result
+          end
+
+          it "doesn't merge the result" do
+            expect(SimpleCov::ResultMerger).not_to receive(:merged_result)
+            SimpleCov.result
+          end
+
+          it "caches its result" do
+            result = SimpleCov.result
+            expect(SimpleCov.result).to be(result)
+          end
+        end
+      end
+
+      context "with merging enabled" do
+        let(:the_merged_result) { double }
+
+        before do
+          allow(SimpleCov).to receive(:use_merging).once.and_return(true)
+          allow(SimpleCov::ResultMerger).to receive(:store_result).once
+          allow(SimpleCov::ResultMerger).to receive(:merged_result).once.and_return(the_merged_result)
+        end
+
+        context "when not running" do
+          before do
+            allow(SimpleCov).to receive(:running).and_return(false)
+          end
+
+          it "merges the result" do
+            expect(SimpleCov.result).to be(the_merged_result)
+          end
+        end
+
+        context "when running" do
+          before do
+            allow(SimpleCov).to receive(:running).and_return(true, false)
+          end
+
+          it "uses the result from Coverage" do
+            expect(Coverage).to receive(:result).once.and_return({})
+            SimpleCov.result
+          end
+
+          it "adds not-loaded-files" do
+            expect(SimpleCov).to receive(:add_not_loaded_files).once.and_return({})
+            SimpleCov.result
+          end
+
+          it "stores the current coverage" do
+            expect(SimpleCov::ResultMerger).to receive(:store_result).once
+            SimpleCov.result
+          end
+
+          it "merges the result" do
+            expect(SimpleCov.result).to be(the_merged_result)
+          end
+
+          it "caches its result" do
+            result = SimpleCov.result
+            expect(SimpleCov.result).to be(result)
+          end
+        end
+      end
+    end
+  end
+end