From df2306cf82c62763142fdc7c7fb21691d17a7f5d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 8 Aug 2019 15:33:40 +1000 Subject: [PATCH 1/5] Lock variants during checkout to avoid race condition It was possible that several people bought the same variant even though there wasn't enough stock for everybody. That resulted in negative stock. --- app/controllers/checkout_controller.rb | 29 ++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 71e9239523..77eef00ed4 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -3,6 +3,8 @@ require 'open_food_network/address_finder' class CheckoutController < Spree::CheckoutController layout 'darkswarm' + prepend_around_filter :lock_variants, only: :update + prepend_before_filter :check_hub_ready_for_checkout prepend_before_filter :check_order_cycle_expiry prepend_before_filter :require_order_cycle @@ -76,6 +78,33 @@ class CheckoutController < Spree::CheckoutController private + # We need locks to avoid a race condition on stock checking. Otherwise we end + # up with negative stock when many people check out at the same time. This + # implementation makes a checkout wait for all other checkouts containing one + # of the order`s 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 different shops. + # + # Ordering the variants by id prevents deadlocks. Plucking the id sends the + # locking query without building Spree::Variant objects. + def lock_variants + # The before action `load_order` handles this error state: + return yield if current_order.nil? + + ActiveRecord::Base.transaction do + # Prevent another checkout from completing the order at the same time: + current_order.lock! + + # Prevent any other checkouts of the same line items at the same time: + variant_ids = current_order.line_items.select(:variant_id) + Spree::Variant.where(id: variant_ids).order(:id).lock.pluck(:id) + + yield + end + end + def set_default_bill_address if params[:order][:default_bill_address] new_bill_address = @order.bill_address.clone.attributes From ec1b5a7a92e71b4020feabd700f022c74ec12846 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 12 Nov 2019 19:52:29 +1100 Subject: [PATCH 2/5] Test concurrent checkouts When two people tried to buy the same item at the same time, it was possible to oversell the item and end up with negative stock. Parallel checkouts could also lead to other random failures. This spec is testing that scenario by starting two threads which would run into a race condition unless they use effective synchronisation. The added spec fails if the synchronisation is removed from the CheckoutController. --- spec/controllers/checkout_controller_spec.rb | 76 ++++++++++++++++++++ spec/spec_helper.rb | 1 + 2 files changed, 77 insertions(+) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index b67a23b8ac..b1c01f90f2 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -266,4 +266,80 @@ describe CheckoutController, type: :controller do controller.send(:update_failed) end end + + # 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 "concurrency", concurrency: true do + # Re-defining test data because we need full data without doubles. + # :order_cycle has suppliers, distributors and products. + 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 first one will wait at the breakpoint. + # The second waits for the first one if it uses correct synchronisation. + # If the checkout code wouldn't wait, it would load the order, then halt + # at the breakpoint and go into a race with the first thread. + # The result of that race are (random) errors which make this spec fail. + 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 end 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 From dc122a9450658700746ae67a85e062529d1040bd Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 13 Nov 2019 10:25:09 +1100 Subject: [PATCH 3/5] Fix infinite loop in spec The spec was setting the order's state to "complete" but didn't save that state to the database. The new locking mechanism is was reloading the order which loaded the cart state again. And since the order.next method was mocked to just return true, the controller was trying to do that in an infinite loop. --- spec/controllers/checkout_controller_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index b1c01f90f2..3c0738f0b9 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) From 4288428c70cb8bb5b5862d628ee0728dfcbb7418 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 19 Nov 2019 16:21:48 +1100 Subject: [PATCH 4/5] Separating concurrency spec as it's entirely different --- .../checkout_controller_concurrency_spec.rb | 89 +++++++++++++++++++ spec/controllers/checkout_controller_spec.rb | 76 ---------------- 2 files changed, 89 insertions(+), 76 deletions(-) create mode 100644 spec/controllers/checkout_controller_concurrency_spec.rb 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 3c0738f0b9..7c149f6f3b 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -267,80 +267,4 @@ describe CheckoutController, type: :controller do controller.send(:update_failed) end end - - # 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 "concurrency", concurrency: true do - # Re-defining test data because we need full data without doubles. - # :order_cycle has suppliers, distributors and products. - 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 first one will wait at the breakpoint. - # The second waits for the first one if it uses correct synchronisation. - # If the checkout code wouldn't wait, it would load the order, then halt - # at the breakpoint and go into a race with the first thread. - # The result of that race are (random) errors which make this spec fail. - 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 end From 50093c325a178b435b198be1f423127227af7e4b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 19 Nov 2019 18:11:47 +1100 Subject: [PATCH 5/5] Move checkout locking to its own service It gives this complex logic more space and allows for better structure and more comments at the right places. --- app/controllers/checkout_controller.rb | 31 ++------------------ app/services/current_order_locker.rb | 40 ++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 28 deletions(-) create mode 100644 app/services/current_order_locker.rb diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 77eef00ed4..3b3b7f95c8 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -3,7 +3,9 @@ require 'open_food_network/address_finder' class CheckoutController < Spree::CheckoutController layout 'darkswarm' - prepend_around_filter :lock_variants, only: :update + # 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 @@ -78,33 +80,6 @@ class CheckoutController < Spree::CheckoutController private - # We need locks to avoid a race condition on stock checking. Otherwise we end - # up with negative stock when many people check out at the same time. This - # implementation makes a checkout wait for all other checkouts containing one - # of the order`s 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 different shops. - # - # Ordering the variants by id prevents deadlocks. Plucking the id sends the - # locking query without building Spree::Variant objects. - def lock_variants - # The before action `load_order` handles this error state: - return yield if current_order.nil? - - ActiveRecord::Base.transaction do - # Prevent another checkout from completing the order at the same time: - current_order.lock! - - # Prevent any other checkouts of the same line items at the same time: - variant_ids = current_order.line_items.select(:variant_id) - Spree::Variant.where(id: variant_ids).order(:id).lock.pluck(:id) - - yield - end - end - def set_default_bill_address if params[:order][:default_bill_address] new_bill_address = @order.bill_address.clone.attributes 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