From 78c1171e98b7227f6bdd8f76f4c14666fd7fc5ea 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. This has the added benefit of reducing the runtime of at_exit hooks by over 66%, since we now do less than 1/3 the work. 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, so this race condition causes exceptions as seen in #406. In addition to fixing the race condition, also add ` || {}` to make `.stored_data` a bit more robust and protect against an empty .resultset.json. --- lib/simplecov.rb | 21 ++++--- lib/simplecov/result_merger.rb | 27 ++++---- spec/result_merger_spec.rb | 22 ++++--- spec/simplecov_spec.rb | 110 +++++++++++++++++++++++++++++++++ 4 files changed, 153 insertions(+), 27 deletions(-) create mode 100644 spec/simplecov_spec.rb 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 bfaf61ab..098fe731 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