Hi. We''ve recently upgraded to latest edge and found a few bugs. I''ve uploaded fixes of them to http://github.com/alk/rails/commits/master I haven''t posted lighthouse tickets because there are 7 commits and I think it would be inconvenient both for me and for you. If it''s not ok, please let me know. So that I can post lighthouse tickets. Also feel free to cherry-pick. I''ll post remaining fixes via tickets. -- Aliaksey Kandratsenka <alkondratenko@gmail.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 -~----------~----~----~----~------~----~------~--~---
Aliaksey Kandratsenka wrote:> Hi. > > We''ve recently upgraded to latest edge and found a few bugs. > > I''ve uploaded fixes of them to > http://github.com/alk/rails/commits/master > > I haven''t posted lighthouse tickets because there are 7 commits and I > think it would be inconvenient both for me and for you. If it''s not ok, > please let me know. So that I can post lighthouse tickets. Also feel > free to cherry-pick. I''ll post remaining fixes via tickets. >These cover several different areas, and I''d prefer lighthouse tickets in general. Feel free to combine all the checkout ones into one ticket. However with: http://github.com/alk/rails/commit/db5d9aef00703e695335e8357608f7061fda35e9 Reloading isn''t thread safe and *can''t* be due to the way constant definition works in ruby. Why are you doing it in production mode? allow_concurrency should be false for development mode? Is it not? -- Cheers, Koz --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
У Суб, 04/10/2008 у 15:26 +0200, Michael Koziarski піша:> These cover several different areas, and I''d prefer lighthouse tickets > in general. Feel free to combine all the checkout ones into one ticket.Ok, I''ll post tickets.> > However with: > > http://github.com/alk/rails/commit/db5d9aef00703e695335e8357608f7061fda35e9 > > Reloading isn''t thread safe and *can''t* be due to the way constant > definition works in ruby. Why are you doing it in production mode? > allow_concurrency should be false for development mode? Is it not? >The problem is that allow_concurrency mutex doesn''t protect class reloading. And in development mode under webrick when 2 subsequent requests come from different connection we have one thread trying to execute some action and other thread doing #cleanup_application. -- Aliaksey Kandratsenka <alkondratenko@gmail.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 -~----------~----~----~----~------~----~------~--~---
У Суб, 04/10/2008 у 17:05 +0300, Aliaksey Kandratsenka піша:> У Суб, 04/10/2008 у 15:26 +0200, Michael Koziarski піша: > > > These cover several different areas, and I''d prefer lighthouse tickets > > in general. Feel free to combine all the checkout ones into one ticket. > Ok, I''ll post tickets. > >Hi again. Here are the tickets: http://rails.lighthouseapp.com/projects/8994/tickets/1168-patch-dont-quote-decimal-values-for-mysql-it-doesnt-make-sence-and-breaks-in-newer-versions-of-mysql this is required for working with decimal columns on sufficiently new version of mysql. This version is at least 5.0.51 as packaged by debian unstable. http://rails.lighthouseapp.com/projects/8994/tickets/1170-patch-return-processing-lock-to-dispatcher-this-fixes-class-reloading-race-in-development-mode without this class reloading happens concurrently with action processing. I think it needs changelog entry because it removes allow_concurrency again. http://rails.lighthouseapp.com/projects/8994/tickets/1169-patch-fix-race-in-connectionpoolcheckout this is a fix for obvious race in #checkout. This also includes small enchancement to same method. http://rails.lighthouseapp.com/projects/8994/tickets/1171-patch-call-clear_active_connections-in-after_dispatch-to-give-pooled-connections-back fix for rails not returning db connections back to pool. I can only guess why nobody hasn''t run into this before. http://rails.lighthouseapp.com/projects/8994/tickets/1172-patch-made-validates_numericality_of-reject-huge-numbers-which-parse-as-infinity minor change of #validates_numericality_of behaviour with regard to huge numbers http://rails.lighthouseapp.com/projects/8994/tickets/1173-patch-fix-performance-bug-in-attibutemethodsrespond_to-in-handling-of-private-methods this fixes tests time for our application from 5 mins back to 3 mins (as was with rails 2.1). It can be less significant for others, but it''s still an easy fix. -- Aliaksey Kandratsenka <alkondratenko@gmail.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 -~----------~----~----~----~------~----~------~--~---
У Суб, 04/10/2008 у 18:00 +0300, Aliaksey Kandratsenka піша:> http://rails.lighthouseapp.com/projects/8994/tickets/1172-patch-made-validates_numericality_of-reject-huge-numbers-which-parse-as-infinity > minor change of #validates_numericality_of behaviour with regard to huge > numbersDon''t apply this. I think it breaks #validates_numericality_of for big decimals. I think I''ll have to add :float and :need_finite options to satisfy my needs. BTW, how can I mark this as invalid ? -- Aliaksey Kandratsenka <alkondratenko@gmail.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 -~----------~----~----~----~------~----~------~--~---
Aliaksey Kandratsenka wrote:> У Суб, 04/10/2008 у 15:26 +0200, Michael Koziarski піша: > >> These cover several different areas, and I''d prefer lighthouse tickets >> in general. Feel free to combine all the checkout ones into one ticket. > Ok, I''ll post tickets. >> However with: >> >> http://github.com/alk/rails/commit/db5d9aef00703e695335e8357608f7061fda35e9 >> >> Reloading isn''t thread safe and *can''t* be due to the way constant >> definition works in ruby. Why are you doing it in production mode? >> allow_concurrency should be false for development mode? Is it not? >> > The problem is that allow_concurrency mutex doesn''t protect class > reloading. And in development mode under webrick when 2 subsequent > requests come from different connection we have one thread trying to > execute some action and other thread doing #cleanup_application.Sure, but you''ve unconditionally introduced the mutex, it should only be there for allow_concurrency = true. -- Cheers, Koz --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
У Суб, 04/10/2008 у 17:35 +0200, Michael Koziarski піша:> Aliaksey Kandratsenka wrote: > > У Суб, 04/10/2008 у 15:26 +0200, Michael Koziarski піша: > > > >> These cover several different areas, and I''d prefer lighthouse tickets > >> in general. Feel free to combine all the checkout ones into one ticket. > > Ok, I''ll post tickets. > >> However with: > >> > >> http://github.com/alk/rails/commit/db5d9aef00703e695335e8357608f7061fda35e9 > >> > >> Reloading isn''t thread safe and *can''t* be due to the way constant > >> definition works in ruby. Why are you doing it in production mode? > >> allow_concurrency should be false for development mode? Is it not? > >> > > The problem is that allow_concurrency mutex doesn''t protect class > > reloading. And in development mode under webrick when 2 subsequent > > requests come from different connection we have one thread trying to > > execute some action and other thread doing #cleanup_application. > > Sure, but you''ve unconditionally introduced the mutex, it should only be > there for allow_concurrency = true.You mean allow_concurrency = false ? -- Aliaksey Kandratsenka <alkondratenko@gmail.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 -~----------~----~----~----~------~----~------~--~---
> You mean allow_concurrency = false ?... yeah :) Exactly. If you leave that setting there and check it in the dispatcher, it should be fine. -- Cheers, Koz --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
У Суб, 04/10/2008 у 17:44 +0200, Michael Koziarski піша:> > You mean allow_concurrency = false ? > > ... yeah :) Exactly. > > If you leave that setting there and check it in the dispatcher, it > should be fine. >I''ve uploaded new patch. -- Aliaksey Kandratsenka <alkondratenko@gmail.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 -~----------~----~----~----~------~----~------~--~---
> > Reloading isn''tthreadsafeand *can''t* be due to the way constant > > definition works in ruby. Why are you doing it in production mode? > > allow_concurrency should be false for development mode? Is itnot?I''m curious about the "constant definition not being threadsafe" -- is this a thread safe bug in MRI? As a note--I''m still afraid of true multi-thread rails for fear of 1) sometimes methods that are defined magically "appear" in other threads but not the current one--so dynamic methods might err. 2) sometimes sockets send data to the wrong socket. It is truly odd and I have no idea why. I think it''s far less of a problem when you use eventmachine, but still might be there lurking somewhere. Thanks! -=R --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
У Аўт, 21/10/2008 у 23:46 -0700, rogerdpack піша:> > > Reloading isn''tthreadsafeand *can''t* be due to the way constant > > > definition works in ruby. Why are you doing it in production mode? > > > allow_concurrency should be false for development mode? Is itnot? > > I''m curious about the "constant definition not being threadsafe" -- is > this a thread safe bug in MRI? >One of the problems here is that the class which is being defined is visible in other threads. So threads may see partially defined classes. This is very hard to fix. And it''s indeed ruby''s (not just MRI, I believe) problem. In MRI it is possible to stop all other threads (by setting Thread.critical) and Rails could try to solve this problem by holding this flag around #load and #require. But this is not a proper solution, because Thread.critical is implementation-specific feature and sooner or later ruby will need to get rid of it. And it should be noted that even this solution may fail if fancy things is tried during load. For example if it tries to lock some mutex, which is locked by some other thread. -- Aliaksey Kandratsenka <alkondratenko@gmail.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 -~----------~----~----~----~------~----~------~--~---
>> I''m curious about the "constant definition not being threadsafe" -- is >> this a thread safe bug in MRI? >> > One of the problems here is that the class which is being defined is > visible in other threads. So threads may see partially defined classes. > This is very hard to fix. And it''s indeed ruby''s (not just MRI, I > believe) problem.Yes this is the issue exactly, and it''s a design thing, not just some implementation detail. This is why rails 2.2 preloads your application classes.> In MRI it is possible to stop all other threads (by setting > Thread.critical) and Rails could try to solve this problem by holding > this flag around #load and #require. But this is not a proper solution, > because Thread.critical is implementation-specific feature and sooner or > later ruby will need to get rid of it. And it should be noted that even > this solution may fail if fancy things is tried during load. For example > if it tries to lock some mutex, which is locked by some other thread.Thread.critical and friends aren''t in 1.9, so it''d be a mistake for us to build on top of them. Also as you mentioned it''s almost impossible to write deterministic code when you have threads grabbing critical status while other threads have locks.> -- > Aliaksey Kandratsenka <alkondratenko@gmail.com> > > > > >-- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
At 10:35 AM +0300 10/22/08, Aliaksey Kandratsenka wrote:>ì Ä Ú, 21/10/2008 Û 23:46 -0700, rogerdpack Ô¥¯ý: >> > > Reloading isn''tthreadsafeand *can''t* be due to the way constant >> > > definition works in ruby. Why are you doing it in production mode? >> > > allow_concurrency should be false for development mode? Is itnot? >> >> I''m curious about the "constant definition not being threadsafe" -- is >> this a thread safe bug in MRI? >> >One of the problems here is that the class which is being defined is >visible in other threads. So threads may see partially defined classes. >This is very hard to fix. And it''s indeed ruby''s (not just MRI, I >believe) problem. > >In MRI it is possible to stop all other threads (by setting >Thread.critical) and Rails could try to solve this problem by holding >this flag around #load and #require. But this is not a proper solution, >because Thread.critical is implementation-specific feature and sooner or >later ruby will need to get rid of it. And it should be noted that even >this solution may fail if fancy things is tried during load. For example >if it tries to lock some mutex, which is locked by some other thread.fyi: Here''s a pointer to a recent thread among the jruby developers about a similar issue: ''require'' thread safety? http://www.nabble.com/%27require%27-thread-safety--td19988160.html#a19988160 Basically what should require do when the same library is loaded by two or more threads? The problem is that after the first "require ''mylib''" starts (but before it has finished evaluating the code) what should ruby do when a script in a second thread evaluates: "require ''mylib''". They decided to: 2. synchronize against the list of loaded features, such that they may both search for the library but only one will load it; however, this won''t guarantee the library has been *completely* loaded At 12:49 PM -0500 10/16/08, Charles Oliver Nutter wrote:>I think there''s a strong argument that you have no guarantee after calling require and receiving "false" that the library has *completed* loading, only that someone else has *initiated* a load. It''s certainly a bad pattern to be doing requires in many threads, and I don''t personally believe there''s any level of locking or synchronization that would safely guarantee a require that''s started has completed before someone tries to use the code in that library. > >I''d love to be proven wrong, but all the scenarios and solutions proposed so far to cause a require to block are certainly going to be deadlock-prone. I think our best be now is definitely (2), to at least guarantee that a given library can''t be require''d twice, even if we can''t guarantee across threads that once require returns false the target library is ready for use.--~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---