how to test lock mechanism

Question

I have a piece of code where I import a BankAccountTransaction to a BankAccount

 bank_account.with_lock do
   transactions.each do |transaction|
     import(bank_account, transaction)
   end
 end

it works fine but I need to write an RSpec case to it so I can be 100% sure I'm not importing transactions twice.

I wrote the following helper

module ConcurrencyHelper
  def make_concurrent_calls(function, concurrent_calls: 2)
    threads = Array.new(concurrent_calls) do
      thread = Thread.new { function.call }
      thread.abort_on_exception = true

      thread
    end

    threads.each(&:join)
  end
end

and I'm calling it on RSpec

context 'when importing the same transaction twice' do
  subject(:concurrent_calls) { make_concurrent_calls(operation) }

  let!(:operation) { -> { described_class.call(params) } }
  let(:filename) { 'single-transaction-response.xml' }

  it 'creates only one transaction' do
    expect { concurrent_calls }.to change(BankaccountTransaction, :count).by(1)
  end
end

but nothing happens, the test suit gets stuck at this point and no errors are thrown or anything like that.

I've put a debug point (byebug) right after I instantiate the threads and tried to call the function and it runs fine but when I join the threads, nothing happens.

Things I tried so far

• breakpoint before threads.each(&:join) and call the function (works fine)
• breakpoint in the rspec example and debug operation and params (all good)

any more ideas?

Edit

this is my current DatabaseCleaner configuration

RSpec.configure do |config|
  config.before(:suite) do
    DatabaseCleaner.clean_with(:deletion)
  end

  config.before do
    DatabaseCleaner.strategy = :transaction
  end

  config.before(:each, js: true) do
    DatabaseCleaner.strategy = :deletion
  end

  config.before do
    DatabaseCleaner.start
  end

  config.after do
    DatabaseCleaner.clean
  end
end

I still haven't tried to modify the strategy to :deleteion, I will do it as well

Faller

Do you have RSpec configured to use transactional_fixtures? Or are you using some other clean up strategy/tool like DatabaseCleaner? relishapp.com/rspec/rspec-rails/docs/transactions – Pectoralis

our use_transactional_fixtures is set to false and we are using DatabaseCleaner, yes – Faller

what strategy are you using with DatabaseCleaner? Have you tried the deletion strategy? – Pectoralis

not yet, I'll try and let you know! – Faller

What version of Ruby and Rails are you running? – Pectoralis

Answer 1

with_lock is Rails's implementation which we don't need to test. You can use a mock and check if your code calls with_lock. The only trick here is to ensure the transaction gets imported (ie. the code inside with_lock gets executed). RSpec will provide the block which you can call. Below is a snippet of how you can do it - full working implementation can be found here.

describe "#import_transactions" do
  it "runs with lock" do
    # Test if with_lock is getting called
    expect(subject).to receive(:with_lock) do |*_args, &block|
      # block is provided to with_lock method
      # execute the block and test if it creates transactions
      expect { block.call }
        .to change { BankAccountTransaction.count }.from(0).to(2)
    end

    ImportService.new.import_transactions(subject, transactions)
  end
end

Rog

this is quite nice, I haven't thought it this way but it solves half my issue. I still need to find a way to call bank_account.with_lock twice using two different threads, which I was unable to do so far – Faller

Exactly my point! That functionality is of Rails, not yours. You should not test library implementations - only test that your code uses the library in its intended way. – Rog

This is a beautiful spec. Thanks. – Gallium

Answer 2

It looks like there's a couple things going on here.

First, the heart of the issue likely lies in a couple areas:

1. Database Cleaning Strategy: You're using the DatabaseCleaner gem which provide three cleaning strategies: Truncation, Transaction, and Deletion. My guess is that if you're using the Transaction strategy, the lock is never released because the first transaction is never committed and the second thread just waits for it to be released.
2. Database Pool Config: Another possible theory is that the connection pool for your test database is too small. This would mean that one (or both) of your threads are waiting to obtain a database connection. Typically there is a timeout configured for this and if one is set you should see an exception that looks something like this: could not obtain a connection from the pool within 5.000 seconds. To fix this, in your database.yml under test adjust your pool setting. You can inspect the values that are set by looking at:

irb(main):001:0> ActiveRecord::Base.connection.pool.size
=> 5
irb(main):001:0> ActiveRecord::Base.connection.pool.checkout_timeout
=> 5

Second, in the code you provided, unless import modifies the transactions or the bank account that it imports, it appears as if the with_lock will not actually prevent multiple uploads.. it will just ensure that they run sequentially.

You might need to do something like this:

 bank_account.with_lock do
   unimported_transactions.each do |transaction|
     import(bank_account, transaction)
     transaction.mark_as_imported!
   end
 end

Additionally, if import is making some sort of external request, you should be careful around partial failures and rollbacks. (the with_lock wraps all SQL queries within it in a database transaction, and if an exception is thrown it will all rollback on your database but not on the external service)

Pectoralis

about point 1: I updated my question with my current configuration. about 1.2 I doubt it would be because of the pool, It doesn't work even when I run only this spec alone. I can try increase as well but I do think you are right on the database cleaning strategy. about your second point, import actually uses a find_or_create_by so it can be run twice, I just need them to run sequentially – Faller

Answer 3

Another approach is to stub out the active record object and negate the database cleaner completely. Something like this (roughly):

let(:bank_account) { double('bank_account') }

before do
  allow(sweeper).to receive(:each).and_return([bank_account])
  allow(bank_account).to receive(:with_lock).and_yield
end

it 'locks' do
  expect(bank_account).to receive(:with_lock)
  ImportService.new.import_transactions
end

Please note this code example is very rough.

Adigranth