diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 71e9239523..3b3b7f95c8 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -3,6 +3,10 @@ require 'open_food_network/address_finder' class CheckoutController < Spree::CheckoutController layout 'darkswarm' + # We need pessimistic locking to avoid race conditions. + # Otherwise we fail on duplicate indexes or end up with negative stock. + prepend_around_filter CurrentOrderLocker, only: :update + prepend_before_filter :check_hub_ready_for_checkout prepend_before_filter :check_order_cycle_expiry prepend_before_filter :require_order_cycle diff --git a/app/services/current_order_locker.rb b/app/services/current_order_locker.rb new file mode 100644 index 0000000000..c420e4ec0d --- /dev/null +++ b/app/services/current_order_locker.rb @@ -0,0 +1,40 @@ +# Locks a controller's current order including its variants. +# +# It should be used when making major changes like checking out the order. +# It can keep stock checking in sync and prevent overselling of an item. +class CurrentOrderLocker + # This interface follows the ActionController filters convention: + # + # https://guides.rubyonrails.org/action_controller_overview.html#filters + # + def self.around(controller) + lock_order_and_variants(controller.current_order) { yield } + end + + # Locking will not prevent all access to these rows. Other processes are + # only waiting if they try to lock one of these rows as well. + # + # https://api.rubyonrails.org/classes/ActiveRecord/Locking/Pessimistic.html + # + def self.lock_order_and_variants(order) + return yield if order.nil? + + order.with_lock do + lock_variants_of(order) + yield + end + end + private_class_method :lock_order_and_variants + + # There are many places in which stock is stored in the database. Row locking + # on variant level ensures that there are no conflicts even when an item is + # sold through multiple shops. + def self.lock_variants_of(order) + variant_ids = order.line_items.select(:variant_id) + + # Ordering the variants by id prevents deadlocks. Plucking the ids sends + # the locking query without building Spree::Variant objects. + Spree::Variant.where(id: variant_ids).order(:id).lock.pluck(:id) + end + private_class_method :lock_variants_of +end diff --git a/spec/controllers/checkout_controller_concurrency_spec.rb b/spec/controllers/checkout_controller_concurrency_spec.rb new file mode 100644 index 0000000000..6a5fcd8bad --- /dev/null +++ b/spec/controllers/checkout_controller_concurrency_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +# This is the first example of testing concurrency in the Open Food Network. +# If we want to do this more often, we should look at: +# +# https://github.com/forkbreak/fork_break +# +# The concurrency flag enables multiple threads to see the same database +# without isolated transactions. +describe CheckoutController, concurrency: true, type: :controller do + let(:order_cycle) { create(:order_cycle) } + let(:distributor) { order_cycle.distributors.first } + let(:order) { create(:order, order_cycle: order_cycle, distributor: distributor) } + let(:address) { create(:address) } + let(:payment_method) { create(:payment_method, distributors: [distributor]) } + let(:breakpoint) { Mutex.new } + + before do + # Create a valid order ready for checkout: + create(:shipping_method, distributors: [distributor]) + variant = order_cycle.variants_distributed_by(distributor).first + order.line_items << create(:line_item, variant: variant) + + # Set up controller environment: + session[:order_id] = order.id + allow(controller).to receive(:spree_current_user).and_return(order.user) + allow(controller).to receive(:current_distributor).and_return(order.distributor) + allow(controller).to receive(:current_order_cycle).and_return(order.order_cycle) + + # New threads start running straight away. The breakpoint is after loading + # the order and before advancing the order's state and making payments. + breakpoint.lock + allow(controller).to receive(:check_order_for_phantom_fees) do + breakpoint.synchronize {} + end + end + + it "waits for concurrent checkouts" do + # Basic data the user submits during checkout: + address_params = address.attributes.except("id") + order_params = { + "payments_attributes" => [ + { + "payment_method_id" => payment_method.id, + "amount" => order.total + } + ], + "bill_address_attributes" => address_params, + "ship_address_attributes" => address_params, + } + + # Starting two checkout threads. The controller code will determine if + # these two threads are synchronised correctly or run into a race condition. + # + # 1. If the controller synchronises correctly: + # The first thread locks required resources and then waits at the + # breakpoint. The second thread waits for the first one. + # 2. If the controller fails to prevent the race condition: + # Both threads load required resources and wait at the breakpoint to do + # the same checkout action. This will lead to (random) errors. + # + # I observed: + # ActiveRecord::RecordNotUnique: duplicate key value violates unique + # constraint "index_spree_shipments_on_order_id" + # on `INSERT INTO "spree_shipments" ...`. + # + # Or: + # ActiveRecord::InvalidForeignKey: insert or update on table + # "spree_orders" violates foreign key constraint + # "spree_orders_customer_id_fk" + threads = [ + Thread.new { spree_post :update, format: :json, order: order_params }, + Thread.new { spree_post :update, format: :json, order: order_params }, + ] + # Let the threads run again. They should not be in a race condition. + breakpoint.unlock + # Wait for both threads to finish. + threads.each(&:join) + order.reload + + # When the spec passes, both threads have the same result. The user should + # see the order page. This is basically verifying a "double click" + # scenario. + expect(response.status).to eq(200) + expect(response.body).to eq({ path: spree.order_path(order) }.to_json) + expect(order.payments.count).to eq 1 + expect(order.completed?).to be true + end +end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index b67a23b8ac..7c149f6f3b 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -130,6 +130,7 @@ describe CheckoutController, type: :controller do context 'when completing the order' do before do order.state = 'complete' + order.save! allow(order).to receive(:update_attributes).and_return(true) allow(order).to receive(:next).and_return(true) allow(order).to receive(:set_distributor!).and_return(true) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 983a80d9e6..5eb8cad557 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -100,6 +100,7 @@ RSpec.configure do |config| config.before(:suite) { DatabaseCleaner.clean_with :deletion, except: ['spree_countries', 'spree_states'] } config.before(:each) { DatabaseCleaner.strategy = :transaction } config.before(:each, js: true) { DatabaseCleaner.strategy = :deletion, { except: ['spree_countries', 'spree_states'] } } + config.before(:each, concurrency: true) { DatabaseCleaner.strategy = :deletion, { except: ['spree_countries', 'spree_states'] } } config.before(:each) { DatabaseCleaner.start } config.after(:each) { DatabaseCleaner.clean } config.after(:each, js: true) do