diff options
| -rw-r--r-- | arel.gemspec | 1 | ||||
| -rw-r--r-- | arel.gemspec.erb | 1 | ||||
| -rw-r--r-- | lib/arel/visitors/reduce.rb | 5 | ||||
| -rw-r--r-- | lib/arel/visitors/visitor.rb | 5 | ||||
| -rw-r--r-- | test/visitors/test_dispatch_contamination.rb | 49 |
5 files changed, 57 insertions, 4 deletions
diff --git a/arel.gemspec b/arel.gemspec index 3924bd15d2..f0d2f66887 100644 --- a/arel.gemspec +++ b/arel.gemspec @@ -23,4 +23,5 @@ Gem::Specification.new do |s| s.add_development_dependency('minitest', '~> 5.4') s.add_development_dependency('rdoc', '~> 4.0') s.add_development_dependency('rake') + s.add_development_dependency('concurrent-ruby', '~> 1.0') end diff --git a/arel.gemspec.erb b/arel.gemspec.erb index f7ff99469c..58f07898d3 100644 --- a/arel.gemspec.erb +++ b/arel.gemspec.erb @@ -23,4 +23,5 @@ Gem::Specification.new do |s| s.add_development_dependency('minitest', '~> 5.4') s.add_development_dependency('rdoc', '~> 4.0') s.add_development_dependency('rake') + s.add_development_dependency('concurrent-ruby', '~> 1.0') end diff --git a/lib/arel/visitors/reduce.rb b/lib/arel/visitors/reduce.rb index 7948758e2f..1156b780f0 100644 --- a/lib/arel/visitors/reduce.rb +++ b/lib/arel/visitors/reduce.rb @@ -11,9 +11,10 @@ module Arel private def visit object, collector - send dispatch[object.class], object, collector + dispatch_method = dispatch[object.class] + send dispatch_method, object, collector rescue NoMethodError => e - raise e if respond_to?(dispatch[object.class], true) + raise e if respond_to?(dispatch_method, true) superklass = object.class.ancestors.find { |klass| respond_to?(dispatch[klass], true) } diff --git a/lib/arel/visitors/visitor.rb b/lib/arel/visitors/visitor.rb index b96b8238a7..2690c98e3c 100644 --- a/lib/arel/visitors/visitor.rb +++ b/lib/arel/visitors/visitor.rb @@ -27,9 +27,10 @@ module Arel end def visit object - send dispatch[object.class], object + dispatch_method = dispatch[object.class] + send dispatch_method, object rescue NoMethodError => e - raise e if respond_to?(dispatch[object.class], true) + raise e if respond_to?(dispatch_method, true) superklass = object.class.ancestors.find { |klass| respond_to?(dispatch[klass], true) } diff --git a/test/visitors/test_dispatch_contamination.rb b/test/visitors/test_dispatch_contamination.rb index 6422a6dff3..71792594d6 100644 --- a/test/visitors/test_dispatch_contamination.rb +++ b/test/visitors/test_dispatch_contamination.rb @@ -1,8 +1,42 @@ # frozen_string_literal: true require 'helper' +require 'concurrent' module Arel module Visitors + class DummyVisitor < Visitor + def initialize + super + @barrier = Concurrent::CyclicBarrier.new(2) + end + + def visit_Arel_Visitors_DummySuperNode node + 42 + end + + # This is terrible, but it's the only way to reliably reproduce + # the possible race where two threads attempt to correct the + # dispatch hash at the same time. + def send *args + super + rescue + # Both threads try (and fail) to dispatch to the subclass's name + @barrier.wait + raise + ensure + # Then one thread successfully completes (updating the dispatch + # table in the process) before the other finishes raising its + # exception. + Thread.current[:delay].wait if Thread.current[:delay] + end + end + + class DummySuperNode + end + + class DummySubNode < DummySuperNode + end + describe 'avoiding contamination between visitor dispatch tables' do before do @connection = Table.engine.connection @@ -17,6 +51,21 @@ module Arel assert_equal "( TRUE UNION FALSE )", node.to_sql end + + it 'is threadsafe when implementing superclass fallback' do + visitor = DummyVisitor.new + main_thread_finished = Concurrent::Event.new + + racing_thread = Thread.new do + Thread.current[:delay] = main_thread_finished + visitor.accept DummySubNode.new + end + + assert_equal 42, visitor.accept(DummySubNode.new) + main_thread_finished.set + + assert_equal 42, racing_thread.value + end end end end |