Skip to content

Make memoized helpers threadsafe

gitlab-qa-bot requested to merge github/fork/JoshCheek/threadsafe-let-block into master

Created by: JoshCheek

Make memoized is a threadsafe object instead of a hash

Accordingly, rename RSpec::Core::MemoizedHelpers::ContextHookMemoizedHash -> ContextHookMemoized

Motivation

When working in a truly threaded environment (e.g. Rbx and JRuby), you can write a test like the one below, which should pass. But, it will fail sometimes, because two threads request the uninitialized counter concurrently. The first one to receive the value will then be incrementing a counter, that is later overwritten when the second thread's let block returns.

You can verify this by running the code sample below in Rubinius. After some number of attempts, the value will be less than 10k. If you then make the let block a let! block, it will memoize before the threads are created, and the values will be correct again. While this is a reasonable solution, it is incredibly confusing when it arises (I spent a lot of hours trying to find the bug in my code), and requires that the user understand it's something they need to be aware of and guard against.

class Counter
  def initialize
    @mutex = Mutex.new
    @count = 0
  end

  attr_reader :count

  def increment
    @mutex.synchronize { @count += 1 }
  end
end

RSpec.describe Counter do
  let(:counter) { Counter.new }

  it 'increments the count in a threadsafe manner' do
    threads = 10.times.map do
      Thread.new { 1000.times { counter.increment } }
    end
    threads.each &:join
    expect(counter.count).to eq 10_000
  end
end

Decisions

Create an object because only #fetch was being called on the hash, and to make that threadsafe required adding a new method. At that point, there is no value in having it subclass Hash, having to support an entire API that nothing else uses. So just make it an object which wraps a hash and presents a memoized getter/setter. Also named the method in such a way as to make it very clear that it's not a hash, so a dev won't mistakenly think they are working with one when they aren't.

I had initially tried writing the test with Fibers. It was much shorter. But Fibers don't work, because they execute within the same thread, so you can't identify the difference between another thread setting the memoized value that we need to respect, and a call to super() setting the value, which we need to override. (eg https://github.com/rspec/rspec-core/blob/655a52b03fd00c3d64554c4268e8da3ac0bd587c/spec/rspec/core/memoized_helpers_spec.rb#L169)

Merge request reports