Rails core - Apr 2008 - Patch for a thread safety issue in action pack

Hey guys

http://dev.rubyonrails.org/ticket/11540

I''ve written a patch for a race condition I found in
ActionView::TemplateHandlers::Compilable.  And I''m fishing for +1s :P
Actually I''d just like some more people to look over it and leave
feedback.

I''d really like Rails to be thread safe and I totally think
it''s a
doable feat.  I''d like to help out as much as I can so if anyone knows
of any problem areas please point me towards them.


--~--~---------~--~----~------------~-------~--~----~
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''d really like Rails to be thread safe and I totally think
it''s a
>  doable feat.  I''d like to help out as much as I can so if anyone
knows
>  of any problem areas please point me towards them.
I think it''s an achievable feat too,  but I''m a little
hesitant to
just whack mutexes everywhere we find a race condition.  Instead I''d
like to think a little more about the changes which are needed to make
the thing work and rejig the internals to make the task simpler.

I''d suggest a candidate first-task is tidying up the ActiveRecord
allow_concurrency stuff to use a connection pool rather than blindly
establishing a connection per thread.   This would require some pretty
heavy-duty refactorings in connection_specification, but it should be
reasonably straightforward.  If anyone''s interested in starting with
this, I''d be happy to collaborate and host a thread-safety branch on
my github account as a staging area before we include it for ''whatever
comes after 2.1''.


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

On Fri, Apr 11, 2008 at 10:55 PM, Michael Koziarski
<michael@koziarski.com> wrote:
>
>  I''d suggest a candidate first-task is tidying up the ActiveRecord
>  allow_concurrency stuff to use a connection pool rather than blindly
>  establishing a connection per thread.   This would require some pretty
>  heavy-duty refactorings in connection_specification, but it should be
>  reasonably straightforward.  If anyone''s interested in starting
with
>  this, I''d be happy to collaborate and host a thread-safety branch
on
>  my github account as a staging area before we include it for
''whatever
>  comes after 2.1''.
I don''t know if I can start on it right away, but I''m very
interested
in helping with this. In particular, the establish_connection stuff
and the way connections are cached is not very friendly right now to
Java appservers where the connection pooling usually happens
underneath the covers. If possible I''d like to find a solution that
includes the possibility of opening and closing connections around
each request and/or transaction. Opening connections in the appserver
is cheap because it''s just checking out/returning the connection to
the pool. Perhaps any work toward that goal can use a similar
abstraction -- then I can wholesale replace Rails'' own connection pool
with one that connects directly to the Java connection pool.

Cheers,
/Nick

--~--~---------~--~----~------------~-------~--~----~
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 don''t know if I can start on it right away, but I''m
very interested
>  in helping with this. In particular, the establish_connection stuff
>  and the way connections are cached is not very friendly right now to
>  Java appservers where the connection pooling usually happens
>  underneath the covers. If possible I''d like to find a solution
that
>  includes the possibility of opening and closing connections around
>  each request and/or transaction. Opening connections in the appserver
>  is cheap because it''s just checking out/returning the connection
to
>  the pool. Perhaps any work toward that goal can use a similar
>  abstraction -- then I can wholesale replace Rails'' own connection
pool
>  with one that connects directly to the Java connection pool.
I spoke about this a little in edinburgh, but connection_specification
is just screaming for some kind of class to manage all that stuff
instead of a bunch of class methods with conditionals.  Even without
any functional changes that code smells and should be made much easier
to customise.   Not to sound too ''enterprise'' but a simple
''connection
factory'' would make most people''s lives that much easier ;).




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

I have my thread safe branch setup already. I''m just toying around w/
some ideas right now. I''ll actually start working on it when this
Google Code thing kicks off.

http://github.com/josh/rails/commits/thread_safe

On Apr 11, 10:55 pm, "Michael Koziarski" <mich...@koziarski.com>
wrote:
> >  I''d really like Rails to be thread safe and I totally think
it''s a
> >  doable feat.  I''d like to help out as much as I can so if
anyone knows
> >  of any problem areas please point me towards them.
>
> I think it''s an achievable feat too,  but I''m a little
hesitant to
> just whack mutexes everywhere we find a race condition.  Instead
I''d
> like to think a little more about the changes which are needed to make
> the thing work and rejig the internals to make the task simpler.
>
> I''d suggest a candidate first-task is tidying up the ActiveRecord
> allow_concurrency stuff to use a connection pool rather than blindly
> establishing a connection per thread.   This would require some pretty
> heavy-duty refactorings in connection_specification, but it should be
> reasonably straightforward.  If anyone''s interested in starting
with
> this, I''d be happy to collaborate and host a thread-safety branch
on
> my github account as a staging area before we include it for
''whatever
> comes after 2.1''.
>
> --
> 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
-~----------~----~----~----~------~----~------~--~---

> I think it''s an achievable feat too,  but I''m a little
hesitant to
> just whack mutexes everywhere we find a race condition.  Instead
I''d
> like to think a little more about the changes which are needed to make
> the thing work and rejig the internals to make the task simpler.
I agree, mutexes are not the solution for every threading problem, but
I believe for some problems they are the best (most pragmatic)
solution.  There is a non mutex solution to this race condition which
is to use a unique method name for every combination of template-name/
local-assings (currently the method name is only based on the template
name).  To me this is more complex and less elegant than a mutex.

There may be a larger reworking of template compilation that fixes
this race condition.  But I don''t see an issue with fixing it with a
mutex until someone takes on that task.  I''d love to hear if you have
a different or better solution to this though.
> I''d suggest a candidate first-task is tidying up the ActiveRecord
> allow_concurrency stuff to use a connection pool rather than blindly
> establishing a connection per thread.   This would require some pretty
On ActiveRecord, I think it''s great that people want to improve it.
But personally I want to focus on the parts of rails which are not yet
thread safe.  AR is at least somewhat thread-safe already.  Of course
I have no problem with anyone else working on it :)
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---

On Apr 14, 10:35 am, coderrr <okst...@gmail.com>
wrote:
> I agree, mutexes are not the solution for every threading problem, but
> I believe for some problems they are the best (most pragmatic)
> solution.  There is a non mutex solution to this race condition which
> is to use a unique method name for every combination of template-name/
> local-assings (currently the method name is only based on the template
> name).  To me this is more complex and less elegant than a mutex.
>
> There may be a larger reworking of template compilation that fixes
> this race condition.  But I don''t see an issue with fixing it with
a
> mutex until someone takes on that task.  I''d love to hear if you
have
> a different or better solution to this though.
>
> > I''d suggest a candidate first-task is tidying up the
ActiveRecord
> > allow_concurrency stuff to use a connection pool rather than blindly
> > establishing a connection per thread.   This would require some pretty
>
> On ActiveRecord, I think it''s great that people want to improve
it.
> But personally I want to focus on the parts of rails which are not yet
> thread safe.  AR is at least somewhat thread-safe already.  Of course
> I have no problem with anyone else working on it :)
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---

On Apr 14, 10:35 am, coderrr <okst...@gmail.com>
wrote:
> I agree, mutexes are not the solution for every threading problem, but
> I believe for some problems they are the best (most pragmatic)
> solution.  There is a non mutex solution to this race condition which
> is to use a unique method name for every combination of template-name/
> local-assings (currently the method name is only based on the template
> name).  To me this is more complex and less elegant than a mutex.
>
> There may be a larger reworking of template compilation that fixes
> this race condition.  But I don''t see an issue with fixing it with
a
> mutex until someone takes on that task.  I''d love to hear if you
have
> a different or better solution to this though.
Rails should compile and cache all the templates on boot. This means
before any dispatch threads are even run.

BTW, just adding mutexes in patch by patch isn''t really going work.
There is already a mutex that wraps the dispatch by mongrel and
another that AP wraps the dispatch w/. (We need to drop the one in the
mongrel handler). I suggest working on this stuff in a different
branch (aka my thread_safe branch :) until we''ve got it done.

I think the bigger problem is getting your app to be threadsafe. We
should look into adding some helpful development features to find non
threadsafe code. I added a feature to my branch that warns you if you
try to modify class variables through a caccessor if it is not safe. I
made a "Thread.current.safe?" that checks if you are 1) In a thread
and 2) not inside a mutex. (all highly experimental!)
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---

On Mon, Apr 14, 2008 at 6:55 PM, Josh Peek <joshpeek@gmail.com> wrote:
>
>  Rails should compile and cache all the templates on boot. This means
>  before any dispatch threads are even run.
Same thing I talked about in IRC. The challenge here is to deal with
partial locals without falling back to method_missing hack and not
screw up the performance, in case anyone is up for it .
>
>  BTW, just adding mutexes in patch by patch isn''t really going
work.
>  There is already a mutex that wraps the dispatch by mongrel and
>  another that AP wraps the dispatch w/. (We need to drop the one in the
>  mongrel handler). I suggest working on this stuff in a different
>  branch (aka my thread_safe branch :) until we''ve got it done.
>
Agree.
-- 
Cheers!
- Pratik
http://m.onkey.org

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

> Rails should compile and cache all the templates on boot. This means
> before any dispatch threads are even run.

I think this is a great goal although it not necessary to solve the race
condition in template compilation.

> BTW, just adding mutexes in patch by patch isn''t really going
work.
> There is already a mutex that wraps the dispatch by mongrel and
> another that AP wraps the dispatch w/. (We need to drop the one in the
> mongrel handler). I  suggest working on this stuff in a different
> branch (aka my thread_safe branch :) until we''ve got it done.
>
I would agree on dropping mongrel mutex since AP protects the dispatch
already, except for... do some older versions of rails not have this mutex?
In which case we''d have a problem.

What I was planning on doing is attacking thread-safety on a case by case
basis.  We can leave the AP dispatch mutex until all known thread safety
problems are solved, at which point we can remove it.  Is there an issue
with having redundant mutexes until that point?

>
> I think the bigger problem is getting your app to be threadsafe. We
> should look into adding some helpful development features to find non
> threadsafe code. I added a feature to my branch that warns you if you
> try to modify class variables through a caccessor if it is not safe. I
> made a "Thread.current.safe?" that checks if you are 1) In a
thread
> and 2) not inside a mutex. (all highly experimental!)
> >
>
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---

Sorry sent last email too early!

BTW, just adding mutexes in patch by patch isn''t really going
work.
> There is already a mutex that wraps the dispatch by mongrel and
> another that AP wraps the dispatch w/. (We need to drop the one in the
> mongrel handler). I suggest working on this stuff in a different
> branch (aka my thread_safe branch :) until we''ve got it done.
>
I''m totally cool with working on stuff in your branch.

I think the bigger problem is getting your app to be threadsafe.
We
> should look into adding some helpful development features to find non
> threadsafe code. I added a feature to my branch that warns you if you
> try to modify class variables through a caccessor if it is not safe. I
> made a "Thread.current.safe?" that checks if you are 1) In a
thread
> and 2) not inside a mutex. (all highly experimental!)

I think having stuff which helps developers write thread-safe code is great
although I would disagree it is a "bigger" problem.  There is
absolutely no
point for a rails developer to write thread safe code at this point since
rails itself isn''t thread safe.  But hopefully that won''t be
the case soon!

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

> Rails should compile and cache all the templates on boot. This means
> before any dispatch threads are even run.

Also  I forgot to add, I think inline templates would make this an
especially hard goal...

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

>  There may be a larger reworking of template compilation that fixes
>  this race condition.  But I don''t see an issue with fixing it
with a
>  mutex until someone takes on that task.  I''d love to hear if you
have
>  a different or better solution to this though.
I''m not suggesting that there''s a magical rewrite which avoids
the
need for a mutex altogether.  But it would be nice to refactor the
compilation code so that we have a single method with the mutex around
it, and perhaps look at other race conditions in the same space.

I think josh''s thread-safe branch is definitely a place to start
playing around.

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