From d2d517059d744244bc8a28ef3eaa334c1ea7c08e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 12 Jul 2020 02:53:19 +0200 Subject: [PATCH] Fix race condition with ballot lines With two concurrent requests, it's possible to create two ballot lines when only one of them should be created. The reason is the code validating the line is not thread safe: ``` if ballot.amount_available(investment.heading) < investment.price.to_i errors.add(:money, "insufficient funds") end ``` If the second request executes this code after the first request has executed it but before the first request has saved the record to the database, both records will pass this validation and both will be saved to the database. So we need to introduce a lock. Now when the second request tries to lock the ballot, it finds it's already locked by the first request, and will wait for the transaction of the first request to finish before checking whether there are sufficient funds. Note we need to disable transactions during the test; otherwise the second thread will wait for the first one to finish. Also note that we need to update a couple of tests because records are reloaded when they're locked. In one case, reloading the ballot causes `ballot.user` to be `nil`, since the user is hidden. So we hide the user after creating all its associated records (which is the scenario that would take place in real life). In the other case, reloading the ballot causes `ballot.user` to reload as well. So we need to reload the user object used in the test too so it gets the updates done on `ballot.user`. I haven't been able to reproduce this behavior in a system test. The following test works with Rails 5.0, but it stopped working when we moved to system tests in commit 9427f014. After that commit, for reasons I haven't been able to debug (reintroducing truncation with DatabaseClaner didn't seem to affect this test, and neither did increasing the number of threads in Puma), the two AJAX requests executed here are no longer simultaneous; the second request waits for the first one to finish. scenario "Race conditions with simultaneous requests", :js do allow_any_instance_of(Budget::Ballot::Line).to receive(:check_sufficient_funds) do |object| allow(object).to receive(:check_sufficient_funds).and_call_original object.check_sufficient_funds sleep 0.3 end ["First", "Second"].each do |title| create(:budget_investment, :selected, heading: california, price: california.price, title: title ) end login_as(user) visit budget_investments_path(budget, heading_id: california.id) within(".budget-investment", text: "First") { click_link "Vote" } within(".budget-investment", text: "Second") { click_link "Vote" } expect(page).to have_link "Remove vote" expect(Budget::Ballot::Line.count).to eq 1 end --- Gemfile | 2 +- app/models/budget/ballot/line.rb | 1 + spec/models/budget/ballot/line_spec.rb | 14 +++++++++++++- spec/models/budget/stats_spec.rb | 4 +++- spec/spec_helper.rb | 9 +++++++++ 5 files changed, 27 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 6b90aebc9..30de89deb 100644 --- a/Gemfile +++ b/Gemfile @@ -68,6 +68,7 @@ end group :development, :test do gem "bullet", "~> 5.9.0" gem "byebug", "~> 11.1.1" + gem "database_cleaner", "~> 1.7.0" gem "factory_bot_rails", "~> 4.8.2" gem "faker", "~> 1.8.7" gem "i18n-tasks", "~> 0.9.29" @@ -94,7 +95,6 @@ group :development do gem "capistrano-rails", "~> 1.4.0", require: false gem "capistrano3-delayed-job", "~> 1.7.3" gem "capistrano3-puma", "~> 4.0.0" - gem "database_cleaner", "~> 1.7.0" gem "erb_lint", require: false gem "github_changelog_generator", "~> 1.15.0" gem "mdl", "~> 0.5.0", require: false diff --git a/app/models/budget/ballot/line.rb b/app/models/budget/ballot/line.rb index f60796547..091ceaea7 100644 --- a/app/models/budget/ballot/line.rb +++ b/app/models/budget/ballot/line.rb @@ -19,6 +19,7 @@ class Budget after_save :store_user_heading def check_sufficient_funds + ballot.lock! errors.add(:money, "insufficient funds") if ballot.amount_available(investment.heading) < investment.price.to_i end diff --git a/spec/models/budget/ballot/line_spec.rb b/spec/models/budget/ballot/line_spec.rb index a4a164cd8..2c3c8f1e4 100644 --- a/spec/models/budget/ballot/line_spec.rb +++ b/spec/models/budget/ballot/line_spec.rb @@ -26,6 +26,18 @@ describe Budget::Ballot::Line do investment.update!(price: heading.price - 1) expect(ballot_line).to be_valid end + + it "validates sufficient funds when creating lines at the same time", :race_condition do + investment.update!(price: heading.price) + other_investment = create(:budget_investment, :selected, price: heading.price, heading: heading) + other_line = build(:budget_ballot_line, ballot: ballot, investment: other_investment) + + [ballot_line, other_line].map do |line| + Thread.new { line.save } + end.each(&:join) + + expect(Budget::Ballot::Line.count).to be 1 + end end describe "Selectibility" do @@ -49,7 +61,7 @@ describe Budget::Ballot::Line do create(:budget_ballot_line, ballot: ballot, investment: investment) - expect(user.balloted_heading_id).to eq(investment.heading.id) + expect(user.reload.balloted_heading_id).to eq(investment.heading.id) end end diff --git a/spec/models/budget/stats_spec.rb b/spec/models/budget/stats_spec.rb index 93c831d7a..c80aff929 100644 --- a/spec/models/budget/stats_spec.rb +++ b/spec/models/budget/stats_spec.rb @@ -10,11 +10,13 @@ describe Budget::Stats do let!(:author_and_voter) { create(:user, :hidden, votables: [investment]) } let!(:voter) { create(:user, votables: [investment]) } let!(:voter_and_balloter) { create(:user, votables: [investment], ballot_lines: [investment]) } - let!(:balloter) { create(:user, :hidden, ballot_lines: [investment]) } + let!(:balloter) { create(:user, ballot_lines: [investment]) } let!(:poll_balloter) { create(:user, :level_two) } let!(:non_participant) { create(:user, :level_two) } before do + balloter.hide + create(:budget_investment, :selected, budget: budget, author: author_and_voter) create(:poll_voter, :from_booth, user: poll_balloter, budget: budget) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 39266a920..3aeee3aae 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -30,6 +30,15 @@ RSpec.configure do |config| Setting["feature.user.skip_verification"] = nil end + config.around(:each, :race_condition) do |example| + self.use_transactional_tests = false + example.run + self.use_transactional_tests = true + + DatabaseCleaner.clean_with(:truncation) + Rails.application.load_seed + end + config.before(:each, type: :system) do Capybara::Webmock.start end