[threadsafe] Arel ToSql visitor is not threadsafe

rubyonrails-core
1

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

2

Hello,

Have you got any tests for the same?

Thanks.

Anuj

3

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

4

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

5

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!

6

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.

7

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

8

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

9

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

10

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!