Hi.
We've recently upgraded to latest edge and found a few bugs.
I've uploaded fixes of them to
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 wrote:
Hi.
We've recently upgraded to latest edge and found a few bugs.
I've uploaded fixes of them to Commits · alk/rails · GitHub
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:
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?
У Суб, 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:
Return processing lock to dispatcher. This fixes class reloading race… · alk/rails@db5d9ae · GitHub
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.
У Суб, 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.
У Суб, 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 numbers
Don'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 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:
Return processing lock to dispatcher. This fixes class reloading race… · alk/rails@db5d9ae · GitHub
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.
У Суб, 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: >> >> Return processing lock to dispatcher. This fixes class reloading race… · alk/rails@db5d9ae · GitHub >> >> 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 ?
You mean allow_concurrency = false ?
... yeah 
Exactly.
If you leave that setting there and check it in the dispatcher, it should be fine.
У Суб, 04/10/2008 у 17:44 +0200, Michael Koziarski піша:
> You mean allow_concurrency = false ?
... yeah
If you leave that setting there and check it in the dispatcher, it should be fine.
I've uploaded new patch.
> 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
У Аўт, 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.
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.
ì ÄÚ, 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/'require'-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