a bunch of fixes

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 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?

У Суб, 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.

У Суб, 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:

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.

У Суб, 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 ?

You mean allow_concurrency = false ?

... yeah :slight_smile: 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 :slight_smile: Exactly.

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