Make ToSql more thread safe.

Because the ToSql visitor instance is shared across all threads, there is a
race condition around column types for binary nodes. It's possible, for instance,
to end up with ActiveRecord converting a string value in the final SQL to an
integer during heavy concurrent operations.

2 files changed, 21 insertions, 5 deletions

diff --git a/lib/arel/visitors/to_sql.rb b/lib/arel/visitors/to_sql.rb
index 73319364d5..4de497a17c 100644
--- a/lib/arel/visitors/to_sql.rb
+++ b/lib/arel/visitors/to_sql.rb
@@ -8,14 +8,13 @@ module Arel
         @engine         = engine
         @connection     = nil
         @pool           = nil
-        @last_column    = nil
         @quoted_tables  = {}
         @quoted_columns = {}
       end
 
       def accept object
-        @last_column = nil
-        @pool        = @engine.connection_pool
+        Thread.current[:arel_visitors_to_sql_last_column] = nil
+        @pool = @engine.connection_pool
         @pool.with_connection do |conn|
           @connection = conn
           super
@@ -356,7 +355,7 @@ key on UpdateManager using UpdateManager#key=
       end
 
       def visit_Arel_Attributes_Attribute o
-        @last_column = column_for o
+        Thread.current[:arel_visitors_to_sql_last_column] = column_for o
         join_name = o.relation.table_alias || o.relation.name
         "#{quote_table_name join_name}.#{quote_column_name o.name}"
       end
@@ -374,7 +373,7 @@ key on UpdateManager using UpdateManager#key=
       alias :visit_Bignum                :literal
       alias :visit_Fixnum                :literal
 
-      def quoted o; quote(o, @last_column) end
+      def quoted o; quote(o, Thread.current[:arel_visitors_to_sql_last_column]) end
 
       alias :visit_ActiveSupport_Multibyte_Chars :quoted
       alias :visit_ActiveSupport_StringInquirer  :quoted
diff --git a/test/visitors/test_to_sql.rb b/test/visitors/test_to_sql.rb
index 1d5f89280b..8084e6441c 100644
--- a/test/visitors/test_to_sql.rb
+++ b/test/visitors/test_to_sql.rb
@@ -1,5 +1,9 @@
 require 'helper'
 
+class Arel::Visitors::ToSql
+  def last_column; Thread.current[:arel_visitors_to_sql_last_column] || @last_column; end
+end
+
 module Arel
   module Visitors
     describe 'the to_sql visitor' do
@@ -9,6 +13,19 @@ module Arel
         @attr = @table[:id]
       end
 
+      it "should be thread safe around usage of last_column" do
+        visit_integer_column = Thread.new do
+          Thread.stop
+          @visitor.send(:visit_Arel_Attributes_Attribute, @attr)
+        end
+
+        @visitor.accept(@table[:name])
+        assert_equal(:string, @visitor.last_column.type)
+        visit_integer_column.run
+        visit_integer_column.join
+        assert_equal(:string, @visitor.last_column.type)
+      end
+
       it 'should not quote sql literals' do
         node = @table[Arel.star]
         sql = @visitor.accept node