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 -- 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 rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Hello, Have you got any tests for the same? Thanks. Anuj On 5 May 2011 02:32, Ketan Padegaonkar <ketanpadegaonkar@gmail.com> wrote:> 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 > > -- > 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 rubyonrails-core@googlegroups.com. > To unsubscribe from this group, send email to > rubyonrails-core+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/rubyonrails-core?hl=en. > >-- Anuj DUTTA -- 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 rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Ketan Padegaonkar
2011-May-05 13:39 UTC
Re: [threadsafe] Arel ToSql visitor is not threadsafe
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 On Thu, May 5, 2011 at 6:06 AM, Anuj Dutta <dutta.anuj@googlemail.com> wrote:> Hello, > > Have you got any tests for the same? > > Thanks. > > Anuj > > > On 5 May 2011 02:32, Ketan Padegaonkar <ketanpadegaonkar@gmail.com> wrote: >> >> 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 >> >> -- >> 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 rubyonrails-core@googlegroups.com. >> To unsubscribe from this group, send email to >> rubyonrails-core+unsubscribe@googlegroups.com. >> For more options, visit this group at >> http://groups.google.com/group/rubyonrails-core?hl=en. >> > > > > -- > Anuj DUTTA > > -- > 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 rubyonrails-core@googlegroups.com. > To unsubscribe from this group, send email to > rubyonrails-core+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/rubyonrails-core?hl=en. >-- 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 rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
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 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.... > Given a high enough number on the loop that creates threads, this test > 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 > > > On 5 May 2011 02:32, Ketan Padegaonkar <ketanpadegaon...@gmail.com> wrote: > > >> 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/52dc69f5bebebb75b7b3d7bd42e839f2...) > >> 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 > > >> -- > >> 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 rubyonrails-core@googlegroups.com. > >> To unsubscribe from this group, send email to > >> rubyonrails-core+unsubscribe@googlegroups.com. > >> For more options, visit this group at > >>http://groups.google.com/group/rubyonrails-core?hl=en. > > > -- > > Anuj DUTTA > > > -- > > 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 rubyonrails-core@googlegroups.com. > > To unsubscribe from this group, send email to > > rubyonrails-core+unsubscribe@googlegroups.com. > > For more options, visit this group at > >http://groups.google.com/group/rubyonrails-core?hl=en.-- 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 rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Aaron Patterson
2011-May-06 18:41 UTC
Re: [threadsafe] Arel ToSql visitor is not threadsafe
On Wed, May 04, 2011 at 06:32:52PM -0700, Ketan Padegaonkar wrote:> 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?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
2011-May-10 15:55 UTC
Re: [threadsafe] Arel ToSql visitor is not threadsafe
On Thu, May 05, 2011 at 06:39:37AM -0700, Ketan Padegaonkar wrote:> 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. -- Aaron Patterson http://tenderlovemaking.com/
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 On May 10, 8:55 am, Aaron Patterson <aa...@tenderlovemaking.com> wrote:> On Thu, May 05, 2011 at 06:39:37AM -0700, Ketan Padegaonkar wrote: > > Hi Anuj, > > > One of my colleagues put some tests here: > >https://github.com/wpc/arel/commit/1b51481487466510189936d9613ceba8a3.... > > 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. > > -- > Aaron Pattersonhttp://tenderlovemaking.com/ > > application_pgp-signature_part > < 1KViewDownload-- 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 rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Aaron Patterson
2011-May-10 23:37 UTC
Re: Re: [threadsafe] Arel ToSql visitor is not threadsafe
On Tue, May 10, 2011 at 01:40:12PM -0700, wpc 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: 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 -- Aaron Patterson http://tenderlovemaking.com/
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 :-) 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:> On Tue, May 10, 2011 at 01:40:12PM -0700, wpc 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: > > https://github.com/wpc/arel/commit/1b51481487466510189936d9613ceba8a3... > > Two other tests fail. When I add the fix provided here: > > https://github.com/ketan/arel/commit/52dc69f5bebebb75b7b3d7bd42e839f2... > > 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/ > > application_pgp-signature_part > < 1KViewDownload-- 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 rubyonrails-core@googlegroups.com. To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
Aaron Patterson
2011-May-11 00:12 UTC
Re: Re: [threadsafe] Arel ToSql visitor is not threadsafe
On Tue, May 10, 2011 at 04:57:47PM -0700, wpc wrote:> 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! -- Aaron Patterson http://tenderlovemaking.com/