a bunch of fixes
Aliaksey Kandratsenka
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 <alkond...@gmail.com>
Michael Koziarski
> 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
Aliaksey Kandratsenka
> 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 <alkond...@gmail.com>
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 <alkond...@gmail.com>
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
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 <alkond...@gmail.com>
Michael Koziarski
Sure, but you've unconditionally introduced the mutex, it should only be
there for allow_concurrency = true.
--
Cheers,
Koz
Aliaksey Kandratsenka
--
Aliaksey Kandratsenka <alkond...@gmail.com>
Michael Koziarski
... yeah :) Exactly.
If you leave that setting there and check it in the dispatcher, it
should be fine.
--
Cheers,
Koz
Aliaksey Kandratsenka
> > You mean allow_concurrency = false ?
>
> ... yeah :) Exactly.
>
> If you leave that setting there and check it in the dispatcher, it
> should be fine.
>
--
Aliaksey Kandratsenka <alkond...@gmail.com>
rogerdpack
> > allow_concurrency should be false for development mode? Is itnot?
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
Aliaksey Kandratsenka
> > > 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?
>
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 <alkond...@gmail.com>
Michael Koziarski
>> 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 <alkond...@gmail.com>
>
>
> >
>
--
Cheers
Koz
Stephen Bannasch
>ì Ä Ú, 21/10/2008 Û 23:46 -0700, rogerdpack Ô¥¯ý:
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.