Skip to content

Commit

Permalink
Fix merging race conditions
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jenseng committed Apr 3, 2017
1 parent d999679 commit 5f5ed37
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 28 deletions.
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
21 changes: 13 additions & 8 deletions lib/simplecov.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
27 changes: 15 additions & 12 deletions lib/simplecov/result_merger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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_|
Expand Down
22 changes: 15 additions & 7 deletions spec/result_merger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down Expand Up @@ -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
Expand Down
110 changes: 110 additions & 0 deletions spec/simplecov_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 5f5ed37

Please sign in to comment.