Forum: Rails-core (closed, excessive spam) Dependencies thread-safety (or not)

81b61875e41eaa58887543635d556fca?d=identicon&s=25 Frederick Cheung (Guest)
on 2008-04-23 11:05
(Received via mailing list)
Are there any claims as to whether Dependencies is thread safe? I was
working on an odd startup problem one of our scripts has and it turns
out that there seems to be a race condition:

2 threads spawn

Thread 1 tries to reference Foo, hits const_missing
Thread 2 tries to reference Foo, hits const_missing
Thread 1 enters require_or_load (in dependencies.rb) and adds foo.rb
to the loaded set
Thread 2 performs the test on line 247 (if file_path && !
loaded.include?(File.expand_path(file_path))) which fails because
loaded does now contain foo.rb
Thread 1 completes loading of foo.rb, Object.const_defined?(:Foo) now
returns true
Thread 2 hits line 253:
elsif (parent = from_mod.parent) && parent != from_mod &&
           ! from_mod.parents.any? { |p| p.const_defined?(const_name) }

which is also false since either from_mod is Object (in which case
parent == from_mod) or from_mod.parents contains Object in which case
the second part of the test
Thread 2 then raises name_error.

I've added some appropriately placed sleep statements into
dependencies.rb to make this happen systematically rather than it just
being blind luck. In my particular case we can probably work around
this by explicitly requiring stuff, but this may be of interest to
people thinking about thread safety in rails.



Fred
0f630dfa170eb15cb9bd041783b6551f?d=identicon&s=25 Tarmo Tänav (Guest)
on 2008-04-23 11:21
(Received via mailing list)
There was a Trac ticket about this:
http://dev.rubyonrails.org/ticket/9155

The problem could probably be fixed with one or more carefully placed
Thread#exclusive blocks. I don't think there is a good reason why
several threads have to be able to load new constants in parallel.
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-23 11:25
(Received via mailing list)
>  Are there any claims as to whether Dependencies is thread safe? I was
>  working on an odd startup problem one of our scripts has and it turns
>  out that there seems to be a race condition:

We actually discussed this in #rails-contrib earlier today.   As it
stands there are absolutely no locks or synchronisation in
dependencies.rb, so it's completely thread-dangerous.  There are a
couple of things we could do:

1) Have a giant 'doing dependencies lock

By wrapping a big lock around load_missing_constant and friends so
only one thread goes about creating classes at any given time.  All
the other threads would have to wait for that lock to be released.
This would probably be a little difficult to test, but not too
difficult.

2) Have an alternative Dependencies mode.

Just load everything up front before we start dispatching requests.

Both these options were discussed in #9155 like tarmo said.  Rather
than Thread#exclusive we could use some specific mutexes so other
threads can go about their other business.  Any takers?



--
Cheers

Koz
81b61875e41eaa58887543635d556fca?d=identicon&s=25 Frederick Cheung (Guest)
on 2008-04-23 11:31
(Received via mailing list)
On 23 Apr 2008, at 10:20, Tarmo Tänav wrote:

>
> There was a Trac ticket about this:
> http://dev.rubyonrails.org/ticket/9155
>
> The problem could probably be fixed with one or more carefully placed
> Thread#exclusive blocks. I don't think there is a good reason why
> several threads have to be able to load new constants in parallel.
>
D'oh, so hell bent in working out exactly why my particular problem
was happening that I didn't think to check trac.

Fred
81b61875e41eaa58887543635d556fca?d=identicon&s=25 Frederick Cheung (Guest)
on 2008-04-23 11:37
(Received via mailing list)
On 23 Apr 2008, at 10:25, Michael Koziarski wrote:

>
>> Are there any claims as to whether Dependencies is thread safe? I was
>> working on an odd startup problem one of our scripts has and it turns
>> out that there seems to be a race condition:
>
> We actually discussed this in #rails-contrib earlier today.   As it
> stands there are absolutely no locks or synchronisation in
> dependencies.rb, so it's completely thread-dangerous.  There are a
> couple of things we could do:
>
Thanks for the confirmation.

> 1) Have a giant 'doing dependencies lock
>
> By wrapping a big lock around load_missing_constant and friends so
> only one thread goes about creating classes at any given time.  All
> the other threads would have to wait for that lock to be released.
> This would probably be a little difficult to test, but not too
> difficult.
>
And loading constants isn't something you spent a lot of time doing
(except perhaps in dev mode) so it shouldn't be much of a bottleneck.
The experiments I did around forcing a systematic failure could
conceivably be used to write a failing test for the current version of
rails. I might have a poke, although there are a few other things that
I've been meaning to tackle that I haven't got round to yet.

Fred
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-23 11:42
(Received via mailing list)
>  Both these options were discussed in #9155 like tarmo said.  Rather
>  than Thread#exclusive we could use some specific mutexes so other
>  threads can go about their other business.  Any takers?

Of course, as commented in trac, defining a new class isn't atomic, so
it's probably Thread#exclusive or nothing.

Thread#exclusive depends on Thread#critical= which isn't considered
kosher, as a result both are gone from 1.9.

So,  unless I'm missing something we need to have an alternative
Dependencies mode if we want really do anything deterministic with
dependencies and threads?


--
Cheers

Koz
77a628df0bd2917d09828dd225a2c0ce?d=identicon&s=25 Hongli Lai (Guest)
on 2008-04-23 11:46
(Received via mailing list)
On Apr 23, 11:25 am, "Michael Koziarski" <mich...@koziarski.com>
wrote:
> 1) Have a giant 'doing dependencies lock
>
> By wrapping a big lock around load_missing_constant and friends so
> only one thread goes about creating classes at any given time.  All
> the other threads would have to wait for that lock to be released.
> This would probably be a little difficult to test, but not too
> difficult.

This could result in a deadlock, if the loaded .rb file hits
const_missing too. As far as I know, Mutex is not recursive.
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-23 11:50
(Received via mailing list)
>  This could result in a deadlock, if the loaded .rb file hits
>  const_missing too. As far as I know, Mutex is not recursive.

I assume we could work around this by keeping track of the thread
which obtained the lock.   However the partially-defined-constant
thing just shoots it all to hell :).


--
Cheers

Koz
81b61875e41eaa58887543635d556fca?d=identicon&s=25 Frederick Cheung (Guest)
on 2008-04-23 11:51
(Received via mailing list)
On 23 Apr 2008, at 10:42, Michael Koziarski wrote:

>
>> Both these options were discussed in #9155 like tarmo said.  Rather
>> than Thread#exclusive we could use some specific mutexes so other
>> threads can go about their other business.  Any takers?
>
> Of course, as commented in trac, defining a new class isn't atomic, so
> it's probably Thread#exclusive or nothing.

Are you worried about
Thread 1 hits const_missing, starts to load foo.rb
Thread 2 references Foo.something, doesn't hit const_missing because
Foo is now defined, but not all of the methods have been added ?
Nasty :-).
Can anything ugly like forcing everyting to be loaded into a temporary
module (so that no one else sees it) and then 'uncloaking' when we're
finished. Sounds like it would just be over the top compared to just
loading everything.

Fred
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-23 12:24
(Received via mailing list)
>  Can anything ugly like forcing everyting to be loaded into a temporary
>  module (so that no one else sees it) and then 'uncloaking' when we're
>  finished. Sounds like it would just be over the top compared to just
>  loading everything.

Yeah, and it's more an issue with ruby rather than something we should
try to work around.   After all:

class Foo
  def Foo.some_class_method
  end
end

So does anyone want to take a stab at a dependencies mechanism which
just loads everything and has regular const_missing behaviour if it
hits something it doesn't understand?



--
Cheers

Koz
81b61875e41eaa58887543635d556fca?d=identicon&s=25 Frederick Cheung (Guest)
on 2008-04-23 12:46
(Received via mailing list)
On 23 Apr 2008, at 11:23, Michael Koziarski wrote:

> class Foo
>  def Foo.some_class_method
>  end
> end
>
> So does anyone want to take a stab at a dependencies mechanism which
> just loads everything and has regular const_missing behaviour if it
> hits something it doesn't understand?

I'll take a shot at it, but I won't have time for a few days

Fred
526d60de6472502bb570a9df2842b33b?d=identicon&s=25 Nick Sieger (Guest)
on 2008-04-23 15:49
(Received via mailing list)
On Wed, Apr 23, 2008 at 4:46 AM, Hongli Lai <honglilai@gmail.com> wrote:
>
>
>  This could result in a deadlock, if the loaded .rb file hits
>  const_missing too. As far as I know, Mutex is not recursive.

Yeah, use Monitor instead which is reentrant.

/Nick
7223c62b7310e164eb79c740188abbda?d=identicon&s=25 Xavier Noria (fxn)
on 2008-04-23 19:35
(Received via mailing list)
On Apr 23, 2008, at 11:25 , Michael Koziarski wrote:

> 2) Have an alternative Dependencies mode.
>
> Just load everything up front before we start dispatching requests.

Does anybody have analized and alternative approach which would
consist on walking load_path before anything to generate Ruby builtin
autoload calls?
6076c22b65b36f5d75c30bdcfb2fda85?d=identicon&s=25 Ezra Zygmuntowicz (Guest)
on 2008-04-23 20:44
(Received via mailing list)
On Apr 23, 2008, at 2:42 AM, Michael Koziarski wrote:
>
> So,  unless I'm missing something we need to have an alternative
> Dependencies mode if we want really do anything deterministic with
> dependencies and threads?
>
>
> --
> Cheers
>
> Koz



  I'd recommend not using Thread.exclusive at all it will end up
limiting rails options on alternate ruby impls. We should either wrap
a mutex around any calls to the Dependencies, or even better in
production mode we should preload all classes before any hits to the
application which would avoid this issue entirely.

Cheers-
- Ezra Zygmuntowicz
-- Founder & Software Architect
-- ezra@engineyard.com
-- EngineYard.com
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-23 23:27
(Received via mailing list)
>  Does anybody have analized and alternative approach which would
>  consist on walking load_path before anything to generate Ruby builtin
>  autoload calls?

I believe the mod_rails guys do this for their system,  it allows the
copy-on-write stuff to only store one copy of all of your apps
classes.  So there are definitely people trying it out.  The
difference is that they're still able to use the const_missing stuff
in the child processes.  As discussed in ticket #9155 there's no way
this can behave in a deterministic manner with multiple threads
requiring classes.

So preloading before spawning threads is really the only safe option,
taking the stuff from Hongli and co as a starting point

--
Cheers

Koz
Efa76b164a7de4a5730e4fa397cc4425?d=identicon&s=25 Michael Koziarski (Guest)
on 2008-04-23 23:27
(Received via mailing list)
>         I'd recommend not using Thread.exclusive at all it will end up
>  limiting rails options on alternate ruby impls. We should either wrap
>  a mutex around any calls to the Dependencies, or even better in
>  production mode we should preload all classes before any hits to the
>  application which would avoid this issue entirely.

Yeah, we don't even have Thread.exclusive in 1.9, so that's out.
Unfortunately mutexes can't save us either, so preloading is the
obvious choice.


--
Cheers

Koz
This topic is locked and can not be replied to.