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.
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.
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?
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.
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.
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?
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 :).
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.
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?
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.
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
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.
