[threadsafe] Arel ToSql visitor is not threadsafe

Hey,

We hit a bug today because Arel::Visitors::ToSql is not threadsafe.
Here is what is happening:

Arel::Visitors::ENGINE_VISITORS is a cache of visitors instances.
These instances are not inherently threadsafe because it contains
state '@last_column', '@connection' that is shared between threads.

The other variables '@pool', '@quoted_tables' and '@quoted_columns'
seem just fine. I'd like to propose a patch
(https://github.com/ketan/arel/commit/52dc69f5bebebb75b7b3d7bd42e839f2db54b88a)
that fixes this using ruby's threadlocal for the last_column and
connection instance variables. Would be great if this was pulled into
master.

Any other thoughts or other recommendations on approaching this?

-- Ketan
studios.thoughtworks.com

Hello,

Have you got any tests for the same?

Thanks.

Anuj

Hi Anuj,

One of my colleagues put some tests here:
https://github.com/wpc/arel/commit/1b51481487466510189936d9613ceba8a32af435.
Given a high enough number on the loop that creates threads, this test
sometimes fails.

-- Ketan
studios.thoughtworks.com

Hi Anuj

For the test, make sure run it using jruby. It fails every single
time.

~/code/arel:<master>? % rvm use jruby@arel
~/code/arel:<master>? % rake
...
Finished tests in 1.045000s, 413.3971 tests/s, 537.7990 assertions/s.

  1) Failure:
test_0006_should_be_threadsafe_on_sql_generation(select manager) [./
test/test_select_manager.rb:954]:
Expected "SELECT \"users\".\"id\" FROM \"users\" WHERE \"users\".\"id
\" = 1 AND \"users\".\"name\" = 'foo'", not "SELECT \"users\".\"id\"
FROM \"users\" WHERE \"users\".\"id\" = 1 AND \"users\".\"name\" = 0".

-- wpc

Sorry to take so long to reply to this. Google groups was bouncing my
emails for some reason.

I think the patch is fine for now. I'd like to eventually cache the
visitor on each connection (as there should only be one connection /
thread) and adjust the AST such that "last_column" is no longer needed.

I'll apply these patches and make a release. Thank you!

Adding this tests breaks the suite even with your proposed patch
applied. Is your patch correct? Are these tests correct?

Can you try applying to master?

Thanks.

Hi, Aaron

After apply patch the test does pass on us. We test it using JRuby
1.6.0 and 1.6.1 on both OSX and Windows. On MRI the test will just
pass anyway even before applying the patch (because of green threads).

Could you post which platform and jruby version are you on along with
the test failure output?

-- WPC

I'm using MRI 1.8.7. When I apply the test case provided here:

  https://github.com/wpc/arel/commit/1b51481487466510189936d9613ceba8a32af435

Two other tests fail. When I add the fix provided here:

  https://github.com/ketan/arel/commit/52dc69f5bebebb75b7b3d7bd42e839f2db54b88a

Still those two tests fail. I've pushed a branch with the two commits
applied here:

  https://github.com/rails/arel/tree/zomg

Here is my output when I run the suite:

  https://gist.github.com/965615

Hi, Aaron:

test_0003_can_make_a_subselect(select manager::backwards
compatibility::as)
test_0001_uses_alias_in_sql(select manager::initialize)
Those two test failed on my MRI 1.8.7 two (but pass on MRI 1.9.2).

But they are not the test I added. The test in the patch is
test_0006_should_be_threadsafe_on_sql_generation(select manager)

I guess those two failing tests are separated issue need look after
anyway :slight_smile:

BTW: the thread safe test in the patch only fails under JRuby (because
MRI dose not have native threads)

-- WPC

Hi, Aaron:

test_0003_can_make_a_subselect(select manager::backwards
compatibility::as)
test_0001_uses_alias_in_sql(select manager::initialize)
Those two test failed on my MRI 1.8.7 two (but pass on MRI 1.9.2).

But they are not the test I added. The test in the patch is
test_0006_should_be_threadsafe_on_sql_generation(select manager)

I guess those two failing tests are separated issue need look after
anyway :slight_smile:

Yup, I just figured this out. Sorry about that!

BTW: the thread safe test in the patch only fails under JRuby (because
MRI dose not have native threads)

I think we could get it to fail on 1.9 as it uses pthreads. I'll see
what I can do!