Skip to content

Commit

Permalink
Add specs for dependency memoization race condition (#40)
Browse files Browse the repository at this point in the history
The Class monkey patch that adds the dependency method creates a
getter method with the same name as the dependency specified. This
method uses a simple check-and-set approach to cache the dependency:

* get the value of an instance variable that caches the dependency
  instance;

* if non-nil, return that value;

* if nil, get the dependency, assign it to the instance variable, then
  return it.

This is intended to ensure that the getter method always returns the
same instance of the dependency. However, this check-and-set is not
thread-safe and can result in different instances being returned
before the instance variable is set.

This adds a pending test for this bug so that we can know when the bug
has been fixed.

Co-authored-by: Tim Regener <[email protected]>
  • Loading branch information
KevinBrowne and timlapluie authored Apr 11, 2024
1 parent 22bd0be commit ee8d43e
Showing 1 changed file with 39 additions and 5 deletions.
44 changes: 39 additions & 5 deletions spec/sinject/class_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,57 @@
let(:bar) { Class.new }

before do
container.register(key: :foo, class: foo)
container.register(key: :bar, class: bar)
container.register(key: :foo, class: foo, singleton: false)
container.register(key: :bar, class: bar, singleton: false)
end

describe 'creates instance methods that' do
it 'have the names specified' do
describe 'creates an instance method that' do
it 'has the name specified' do
expect(subject.instance_methods).not_to include(:foo, :bar)
subject.dependency :foo, :bar
expect(subject.instance_methods).to include(:foo, :bar)
end

it 'return the dependency registered with the same name in the container' do
it 'returns the dependency registered with the same name in the container' do
subject.dependency :foo, :bar
instance = subject.new
expect(instance.foo).to be_an_instance_of(foo)
expect(instance.bar).to be_an_instance_of(bar)
end

it 'returns the same instance each time' do
subject.dependency :foo
instance = subject.new
first = instance.foo
second = instance.foo
expect(first).to be second
end

it 'returns the same instance each time when called concurrently' do
pending('The check and set memoization in the generated getter has no critical sections')
# This test is a bit problematic. It's ultimately
# non-deterministic, though it's likely a reasonably safe
# assumption that the system can spawn at least two threads
# within one second.
container.register(key: :baz, class: Array, singleton: false) do
sleep(1)
[]
end
subject.dependency :baz
instance = subject.new

instances = Set.new.compare_by_identity
mutex = Mutex.new
threads = 2.times.map do
Thread.new do
inst = instance.baz
mutex.synchronize { instances.add(inst) }
end
end
threads.each(&:join)

expect(instances.size).to eq 1
end
end
end
end

0 comments on commit ee8d43e

Please sign in to comment.