[threadsafe] Arel ToSql visitor is not threadsafe
Ketan Padegaonkar
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
Anuj Dutta
Have you got any tests for the same?
Thanks.
Anuj
--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To post to this group, send email to rubyonra...@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-co...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
--
Anuj DUTTA
Ketan Padegaonkar
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
wpc
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
On May 5, 6:39 am, Ketan Padegaonkar <ketanpadegaon...@gmail.com>
wrote:
> Hi Anuj,
>
> One of my colleagues put some tests here:https://github.com/wpc/arel/commit/1b51481487466510189936d9613ceba8a3....
> sometimes fails.
>
> -- Ketan
> studios.thoughtworks.com
>
>
>
>
>
>
>
> On Thu, May 5, 2011 at 6:06 AM, Anuj Dutta <dutta.a...@googlemail.com> wrote:
> > Hello,
>
> > Have you got any tests for the same?
>
> > Thanks.
>
> > Anuj
>
>
> >> 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
>
Aaron Patterson
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!
--
Aaron Patterson
http://tenderlovemaking.com/
Aaron Patterson
> 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.
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.
wpc
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
On May 10, 8:55 am, Aaron Patterson <aa...@tenderlovemaking.com>
wrote:
> > Hi Anuj,
>
> > One of my colleagues put some tests here:
> > sometimes fails.
>
> 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.
>
> --
> Aaron Pattersonhttp://tenderlovemaking.com/
>
> < 1KViewDownload
Aaron Patterson
> 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?
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
wpc
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 :-)
BTW: the thread safe test in the patch only fails under JRuby (because
MRI dose not have native threads)
-- WPC
On May 10, 4:37 pm, Aaron Patterson <aa...@tenderlovemaking.com>
wrote:
> > 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?
>
> I'm using MRI 1.8.7. When I apply the test case provided here:
>
> Two other tests fail. When I add the fix provided here:
>
> 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
>
> --
> Aaron Pattersonhttp://tenderlovemaking.com/
>
> < 1KViewDownload
Aaron Patterson
> 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 :-)
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!