From 6f12dd6993f5cfb1c29a6842efae62a95e933649 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 and synchronize 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::ResultMerger.stored_data` doesn't synchronize its read, so it can see an empty file if another process is writing it. 2. `SimpleCov::ResultMerger.resultset` calls `.stored_data` twice and never caches the result. 3. `SimpleCov.result` doesn't cache the `@result`, so the default `at_exit` behavior causes it to store and merge 3 times. Due to 1. and 2., this is extra bad This can cause the formatter to miss out on coverage data 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 | 55 +++++++++++++---- spec/result_merger_spec.rb | 93 ++++++++++++++++++++++++++-- spec/simplecov_spec.rb | 109 +++++++++++++++++++++++++++++++++ 4 files changed, 252 insertions(+), 26 deletions(-) create mode 100644 spec/simplecov_spec.rb diff --git a/lib/simplecov.rb b/lib/simplecov.rb index 3e3ed5e8..d4247068 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..21df15ba 100644 --- a/lib/simplecov/result_merger.rb +++ b/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 diff --git a/spec/result_merger_spec.rb b/spec/result_merger_spec.rb index bfaf61ab..27850af6 100644 --- a/spec/result_merger_spec.rb +++ b/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 diff --git a/spec/simplecov_spec.rb b/spec/simplecov_spec.rb new file mode 100644 index 00000000..ae3a85f1 --- /dev/null +++ b/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