Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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.
  • Loading branch information...
commit 73ca3932d3669f7a6f9bb722a6abaedb75f66929 1 parent a3d9c39
Damon McCormick + Cameron Walters authored
Showing with 21 additions and 5 deletions.
  1. +4 −5 lib/arel/visitors/to_sql.rb
  2. +17 −0 test/visitors/test_to_sql.rb
View
9 lib/arel/visitors/to_sql.rb
@@ -8,14 +8,13 @@ def initialize engine
@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 @@ def visit_Arel_Nodes_UnqualifiedColumn o
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 @@ def literal o; o end
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
View
17 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 Visitors
@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

1 comment on commit 73ca393

@btatnall

Thanks for the fix. I've started observing strings turn into integers for a number of my queries.

Please sign in to comment.
Something went wrong with that request. Please try again.