Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Autoload thread safety #2080

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

thedarkone commented Dec 7, 2012

Autoload on Rubinius is not thread safe, test gist: https://gist.github.com/5e08c309ad8b69b2a31b

This passes on JRuby 1.7, MRI 2.0 and fails on MRI 1.8, MRI 1.9.3, Rubinius.

I can turn this into rubyspec (targeting 2.0?) if you guys want.

Owner

dbussink commented Dec 6, 2012

@evanphx Since you worked on this, does this ring any immediate bells?

Owner

brixen commented Dec 6, 2012

@thedarkone thanks for the ticket. If you have the time, please write specs. We need specs before we can fix the issue.

Contributor

thedarkone commented Dec 7, 2012

Added a spec.

@brixen brixen commented on an outdated diff Dec 8, 2012

spec/ruby/core/module/autoload_spec.rb
@@ -418,6 +418,91 @@ class ModuleSpecs::Autoload::Z < ModuleSpecs::Autoload::ZZ
t2_exc.should be_nil
end
end
+
+ ruby_version_is "2.0" do
+ describe "(concurrently)" do
+ it "blocks others threads while doing an autoload" do
+ class CyclicBarrier
@brixen

brixen Dec 8, 2012

Owner

This needs to go in a fixture file in an appropriate module scope: http://rubyspec.org/style_guide/

@brixen brixen commented on an outdated diff Dec 8, 2012

spec/ruby/core/module/autoload_spec.rb
+ end
+ end
+ end
+
+ def disable!
+ @mutex.synchronize do
+ @count = -1
+ @cond.broadcast
+ end
+ end
+ end
+
+ mod_count = 20
+ thread_count = 20
+
+ autoloads_dir = tmp('concurrent_autoloads')
@brixen

brixen Dec 8, 2012

Owner

This needs to be in a before action: http://rubyspec.org/style_guide/

@brixen brixen commented on an outdated diff Dec 8, 2012

spec/ruby/core/module/autoload_spec.rb
+ mod_names.each do |mod_name|
+ barrier.await # make sure all threads are ready to race
+ begin
+ Object.const_get(mod_name).foo
+ rescue
+ barrier.disable!
+ raise
+ end
+ end
+ end
+ end.map {|t| t.join}
+ end).should_not raise_error(NoMethodError)
+
+ ScratchPad.recorded.size.should == mod_count
+ ensure
+ rm_r(autoloads_dir)
@brixen

brixen Dec 8, 2012

Owner

This needs to be in an after action: http://rubyspec.org/style_guide/

@brixen brixen commented on an outdated diff Dec 8, 2012

spec/ruby/core/module/autoload_spec.rb
+
+ (lambda do
+ (1..thread_count).map do
+ Thread.new do
+ mod_names.each do |mod_name|
+ barrier.await # make sure all threads are ready to race
+ begin
+ Object.const_get(mod_name).foo
+ rescue
+ barrier.disable!
+ raise
+ end
+ end
+ end
+ end.map {|t| t.join}
+ end).should_not raise_error(NoMethodError)
@brixen

brixen Dec 8, 2012

Owner

Do not use should_not raise_error. Compute a value and make an expectation on that value or compute a state change and make an expectation on state.

@brixen brixen commented on an outdated diff Dec 8, 2012

spec/ruby/core/module/autoload_spec.rb
+ end
+
+ def disable!
+ @mutex.synchronize do
+ @count = -1
+ @cond.broadcast
+ end
+ end
+ end
+
+ mod_count = 20
+ thread_count = 20
+
+ autoloads_dir = tmp('concurrent_autoloads')
+ begin
+ mkdir_p(autoloads_dir)
@brixen

brixen Dec 8, 2012

Owner

This should also be in a before action.

Owner

brixen commented Dec 8, 2012

A much better organization for this spec would be to use explicit scripts in the fixture dir that you run rather than generating them at spec runtime.

Contributor

thedarkone commented Dec 8, 2012

Didn't know about the style guide, will rewrite and re-push.

A much better organization for this spec would be to use explicit scripts in the fixture dir that you run rather than generating them at spec runtime.

In this case this would mean adding 20 almost identical new fixture files. Is that alright?

Owner

brixen commented Dec 8, 2012

@thedarkone you need 20 fixtures to demonstrate thread-safe autoload?

Contributor

thedarkone commented Dec 8, 2012

That's the "beauty" of testing thread-safety issues, I can quite easily not get a NoMethodError error with as little as 10 iterations.

Owner

brixen commented Dec 8, 2012

@thedarkone you may find it helpful to examine how I wrote the Process.daemon specs: https://github.com/rubinius/rubinius/blob/master/spec/ruby/core/process/daemon_spec.rb

Contributor

thedarkone commented Dec 9, 2012

Re-pushed. I'm now using a single fixture file, that I just delete from $LOADED_FEATURES at the beginning of each iteration.

Contributor

stouset commented Mar 28, 2013

Can you add an appropriate fails tag in spec/tags/20/ruby/core/module/autoload_tags.txt? Then I can go ahead and merge it in.

@dbussink dbussink closed this in 8fe5a69 May 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment