Skip to content

Commit

Permalink
Make RSpec::Mocks::Space more thread-safe.
Browse files Browse the repository at this point in the history
It used to be possible for different threads to receive different proxy
objects, potentially a source of bugs such as #380.

This is non-trivial to write a spec for, but you can trivially
demonstrate the problem by adding a `sleep` into the fetch blocks, and
then running:

    o = Object.new
    t = Thread.new do
      RSpec::Mocks.space.proxy_for(o)
    end
    b = RSpec::Mocks.space.proxy_for(o)
    a = t.value

    expect(a).to eq(b)
  • Loading branch information
xaviershay committed Mar 23, 2014
1 parent 12510a9 commit 9674568
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 5 deletions.
24 changes: 24 additions & 0 deletions benchmarks/thread_safety.rb
@@ -0,0 +1,24 @@
$LOAD_PATH.unshift(File.expand_path("../../lib", __FILE__))

require 'benchmark'
require 'rspec/mocks'

Benchmark.bm do |bm|
bm.report("fetching a proxy") do
RSpec::Mocks.with_temporary_scope do
o = Object.new
100000.times {
RSpec::Mocks.space.proxy_for(o)
}
end
end
end

# Without synchronize (not thread-safe):
#
# user system total real
# fetching a proxy 0.120000 0.000000 0.120000 ( 0.141333)
#
# With synchronize (thread-safe):
# user system total real
# fetching a proxy 0.180000 0.000000 0.180000 ( 0.189553)
31 changes: 26 additions & 5 deletions lib/rspec/mocks/space.rb
Expand Up @@ -48,6 +48,8 @@ def initialize
@any_instance_recorders = {}
@constant_mutators = []
@expectation_ordering = OrderGroup.new
@proxy_mutex = new_mutex
@any_instance_mutex = new_mutex
end

def new_scope
Expand Down Expand Up @@ -75,9 +77,11 @@ def constant_mutator_for(name)
end

def any_instance_recorder_for(klass)
id = klass.__id__
any_instance_recorders.fetch(id) do
any_instance_recorder_not_found_for(id, klass)
any_instance_mutex.synchronize do
id = klass.__id__
any_instance_recorders.fetch(id) do
any_instance_recorder_not_found_for(id, klass)
end
end
end

Expand All @@ -86,8 +90,10 @@ def proxies_of(klass)
end

def proxy_for(object)
id = id_for(object)
proxies.fetch(id) { proxy_not_found_for(id, object) }
proxy_mutex.synchronize do
id = id_for(object)
proxies.fetch(id) { proxy_not_found_for(id, object) }
end
end

alias ensure_registered proxy_for
Expand All @@ -98,6 +104,21 @@ def registered?(object)

private

attr_reader :proxy_mutex, :any_instance_mutex

# We don't want to depend on the stdlib ourselves, but if the user is
# using threads then a Mutex will be available to us. If not, we don't
# need to synchronize anyway.
def new_mutex
(defined?(::Mutex) ? ::Mutex : FakeMutex).new
end

class FakeMutex
def synchronize
yield
end
end

def proxy_not_found_for(id, object)
proxies[id] = case object
when NilClass then ProxyForNil.new(@expectation_ordering)
Expand Down

0 comments on commit 9674568

Please sign in to comment.