Forum: Rails-core (closed, excessive spam) Patch for a thread safety issue in action pack

Af4ce9309f8c4f7fc5cb33e7a5b08c64?d=identicon&s=25 coderrr (Guest)
on 2008-04-11 11:12
(Received via mailing list)
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.
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-12 05:56
(Received via mailing list)
>  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
526d60de6472502bb570a9df2842b33b?d=identicon&s=25 Nick Sieger (Guest)
on 2008-04-13 06:34
(Received via mailing list)
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
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-13 06:55
(Received via mailing list)
>  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
9a53e50a06e60acfc4184e1dc3e51213?d=identicon&s=25 Josh Peek (Guest)
on 2008-04-13 17:48
(Received via mailing list)
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>
Cd4f9ba2512f984481b5b00279cee69a?d=identicon&s=25 coderrr (Guest)
on 2008-04-14 17:36
(Received via mailing list)
> 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 :)
9a53e50a06e60acfc4184e1dc3e51213?d=identicon&s=25 Josh Peek (Guest)
on 2008-04-14 19:56
(Received via mailing list)
On Apr 14, 10:35 am, coderrr <okst...@gmail.com> wrote:
> 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!)
A05834e9b5954947eb0ba3b570c47d5e?d=identicon&s=25 Pratik Naik (pratik)
on 2008-04-14 20:04
(Received via mailing list)
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
Cd4f9ba2512f984481b5b00279cee69a?d=identicon&s=25 steve (Guest)
on 2008-04-14 21:07
(Received via mailing list)
> 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?
Cd4f9ba2512f984481b5b00279cee69a?d=identicon&s=25 steve (Guest)
on 2008-04-14 21:15
(Received via mailing list)
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!
Cd4f9ba2512f984481b5b00279cee69a?d=identicon&s=25 steve (Guest)
on 2008-04-14 21:37
(Received via mailing list)
> 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...
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-17 10:08
(Received via mailing list)
>  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
This topic is locked and can not be replied to.