From 5f5ed37fef67599de81e0cbfcddb81b41467fe31 Mon Sep 17 00:00:00 2001 From: Jon Jensen Date: Mon, 3 Apr 2017 13:32:14 -0600 Subject: [PATCH] Fix merging race conditions Ensure we cache appropriately so we don't merge more than once or do unsafe reads of the resultset. See https://gist.github.com/jenseng/62465f674f8c02de09ef776f23d4dca4 for simple repro script. Basically there are 3 main problems when using merging: 1. `SimpleCov.result` doesn't cache the `@result`, so the default `at_exit` behavior causes it to store and merge 3 times. 2. `SimpleCov::ResultMerger.resultset` calls `.stored_data` twice. 3. `SimpleCov::ResultMerger.merged_result` doesn't synchronize or use a cached `.resultset`, so a concurrent `.store_result` call can cause us to read an empty file. This can cause the formatter to miss out on coverage data in our formatters and/or get the wrong values for covered percentages. Furthermore, if you use OJ, `JSON.parse("") -> nil`, which means `.resultset` can be nil, causing exceptions as seen in #406. Also ping rubocop even more precisely, as 0.48.1+ fails on existing code. --- Gemfile | 2 +- lib/simplecov.rb | 21 ++++--- lib/simplecov/result_merger.rb | 27 ++++---- spec/result_merger_spec.rb | 22 ++++--- spec/simplecov_spec.rb | 110 +++++++++++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 28 deletions(-) create mode 100644 spec/simplecov_spec.rb diff --git a/Gemfile b/Gemfile index 5bc43a6e..1549d004 100644 --- a/Gemfile +++ b/Gemfile @@ -29,7 +29,7 @@ group :test do gem "cucumber" gem "phantomjs", "~> 2.1" gem "poltergeist" - gem "rubocop", "~> 0.48.0" unless RUBY_VERSION.start_with?("1.") + gem "rubocop", "0.48.0" unless RUBY_VERSION.start_with?("1.") gem "test-unit" end gem "json", RUBY_VERSION.start_with?("1.") ? "~> 1.8" : "~> 2.0" diff --git a/lib/simplecov.rb b/lib/simplecov.rb index 3e3ed5e8..eca6ed98 100644 --- a/lib/simplecov.rb +++ b/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 diff --git a/lib/simplecov/result_merger.rb b/lib/simplecov/result_merger.rb index 58f0dd47..9311530d 100644 --- a/lib/simplecov/result_merger.rb +++ b/lib/simplecov/result_merger.rb @@ -17,17 +17,20 @@ def resultset_writelock File.join(SimpleCov.coverage_path, ".resultset.json.lock") end - # Loads the cached resultset from JSON and returns it as a Hash - def resultset - if stored_data - begin - JSON.parse(stored_data) - rescue - {} - end - else - {} - end + # Loads the cached resultset from JSON and returns it as a Hash, + # caching it for subsequent accesses. + def resultset(force_reload = false) + @resultset = nil if force_reload + + @resultset ||= if (data = stored_data) + begin + JSON.parse(data) || {} + rescue + {} + end + else + {} + end end # Returns the contents of the resultset cache as a string or if the file is missing or empty nil @@ -78,7 +81,7 @@ def merged_result def store_result(result) File.open(resultset_writelock, "w+") do |f| f.flock(File::LOCK_EX) - new_set = resultset + new_set = resultset(true) command_name, data = result.to_hash.first new_set[command_name] = data File.open(resultset_path, "w+") do |f_| diff --git a/spec/result_merger_spec.rb b/spec/result_merger_spec.rb index 8bc6c9d6..7521aea9 100644 --- a/spec/result_merger_spec.rb +++ b/spec/result_merger_spec.rb @@ -4,7 +4,6 @@ describe SimpleCov::ResultMerger do 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,14 +72,23 @@ expect(SimpleCov::ResultMerger.results.length).to eq(1) end end + end + end - context "with merging disabled" do - before { SimpleCov.use_merging false } + 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 equal(set) + end + end - it "returns nil for SimpleCov.result" do - expect(SimpleCov.result).to be_nil - end - end + describe ".resultset" do + it "caches by default" do + set = SimpleCov::ResultMerger.resultset + new_set = SimpleCov::ResultMerger.resultset + expect(new_set).to equal(set) end end end diff --git a/spec/simplecov_spec.rb b/spec/simplecov_spec.rb new file mode 100644 index 00000000..d26669ac --- /dev/null +++ b/spec/simplecov_spec.rb @@ -0,0 +1,110 @@ +require "helper" + +if SimpleCov.usable? + describe SimpleCov do + describe ".result" do + before do + SimpleCov.clear_result! + allow(Coverage).to receive(:result).once.and_return({}) + allow(SimpleCov).to receive(:add_not_loaded_files).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({}) + SimpleCov.result + 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).to receive(:store_result).never + SimpleCov.result + end + + it "doesn't merge the result" do + expect(SimpleCov::ResultMerger).to receive(:merged_result).never + SimpleCov.result + end + + it "caches its result" do + result = SimpleCov.result + expect(SimpleCov.result).to equal(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 equal(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 equal(the_merged_result) + end + + it "caches its result" do + result = SimpleCov.result + expect(SimpleCov.result).to equal(result) + end + end + end + end + end +end