Bug #4181

Backport Ruby 1.9 singleton.rb, since 1.8's is not thread-safe

Added by Charles Nutter over 3 years ago. Updated 11 months ago.

[ruby-core:33796]
Status:Closed
Priority:Normal
Assignee:Shyouhei Urabe
Category:-
Target version:Ruby 1.8.7
ruby -v:Any Ruby 1.8 version

Description

=begin
Ruby 1.9 modified singleton.rb by eliminating much of the lazy init logic, using a real mutex instead of Thread.critical, and eliminating the redefinition of "instance" on first call. None of these changes have been backported into a 1.8 release, which means all 1.8 releases have a broken singleton.rb.

The following script breaks under any version of 1.8:

require 'singleton'
$jruby = RUBY_PLATFORM =~ /java/
require 'jruby/synchronized' if $jruby

loop do
$inits = []
$inits.extend JRuby::Synchronized if $jruby
classes = []
1000.times do
classes << Class.new do
include Singleton
end
end

(0..10).map do
  Thread.new do
    classes.each do |cls|
      cls.instance
    end
  end
end.map(&:join)
puts "loop completed"

end

Results:

~/projects/jruby ➔ ruby -v singletonkiller.rb
ruby 1.8.7 (2010-08-16 patchlevel 302) [i686-darwin10.4.0]
loop completed
loop completed
loop completed
loop completed
loop completed
loop completed
loop completed
loop completed
loop completed
singleton
killer.rb:18: undefined method instance' for #<Class:0x1001896a0> (NoMethodError)
from singleton_killer.rb:1:in
join'
from singletonkiller.rb:1:in to_proc'
from singleton_killer.rb:21:in
map'
from singleton
killer.rb:21
from singletonkiller.rb:5:in `loop'
from singleton
killer.rb:5

~/projects/jruby ➔ ruby -v singletonkiller.rb
ruby 1.8.7 (2009-06-12 patchlevel 174) [universal-darwin10.0]
loop completed
loop completed
loop completed
loop completed
singleton
killer.rb:18: undefined method instance' for #<Class:0x100348c70> (NoMethodError)
from singleton_killer.rb:1:in
join'
from singletonkiller.rb:1:in to_proc'
from singleton_killer.rb:21:in
map'
from singleton
killer.rb:21
from singletonkiller.rb:5:in `loop'
from singleton
killer.rb:5

This can lead to lazy failures in any library that uses singleton.rb. See also this commit to Nokogiri, where they had to stop using Singleton because of this issue:

https://github.com/tenderlove/nokogiri/commit/5eb036e39ea85a8e12eebee11bc5086b0e4ce6e3
=end

History

#1 Updated by Charles Nutter over 3 years ago

=begin
I recorded a short screencast explaining the problem with 1.8's singleton.rb and how 1.9's fixes it.

Basically, because the klass.instance method gets redefined, there's a chance for another thread to attempt to call klass.instance and fail at that exact moment. The issue could potentially be fixed by expanding the critical section over the redefinition of klass.instance, but that would likely require encompassing the actual singleton object creation. In my opinion, that extends the critical section too far. Backporting the 1.9 version, which uses a class-local mutex and no method redefinition, would be best.

http://jruby.headius.com/~headius/Broken%20singleton%20in%201.8.mov
=end

#2 Updated by Charles Nutter over 3 years ago

=begin
FYI, JRuby 1.6 will ship with 1.9's singleton.rb for 1.8 mode, since it seems like the best option for fixing the (broken) 1.8 version.
=end

#3 Updated by Yui NARUSE over 3 years ago

=begin
You mean we should copy 1.9's singleton.rb to 1.8 except "require 'thread'"?
=end

#4 Updated by Charles Nutter over 3 years ago

=begin
require 'thread' is required to get Mutex, which the 1.9 version uses to safely define the singleton. I think the requirement that singleton.rb requires thread.rb is ok.
=end

#5 Updated by Hiroshi Nakamura almost 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to Hiroshi Nakamura
  • Target version set to Ruby 1.8.7

#6 Updated by Hiroshi Nakamura almost 3 years ago

r4424 at 2003-08-03 is the bad commit.

Shyouhei, can I commit this?

Index: lib/singleton.rb
===================================================================
--- lib/singleton.rb (revision 32126)
+++ lib/singleton.rb (working copy)
@@ -94,9 +94,12 @@
@instance = new
ensure
if @instance
- class <<self
- removemethod :instance
- def instance; @
instance_ end
+ # Check method existence to reduce redefinition warning.
+ unless self.respondto?(:instance)
+ # It's doing test-then-set without a lock so there's a race to
+ # override existing method that causes the redefinition warning.
+ # We allow it since method redefinition is thread-safe for calling.
+ def self.instance; @
instance_ end
end
else
@instance = nil # failed instance creation
@@ -111,9 +114,9 @@
@instance = new
ensure
if @instance
- class <<self
- removemethod :instance
- def instance; @
instance_ end
+ # See the comment for self.instance above.
+ unless self.respondto?(:instance)
+ def self.instance; @
instance_ end
end
else
@instance = nil

#7 Updated by Hiroshi Nakamura almost 3 years ago

Hmm. The redmine cannot render my patch correctly. Here's a patch. https://gist.github.com/1037082

#8 Updated by Hiroshi Nakamura almost 3 years ago

  • Priority changed from Immediate to Normal

Hope this would be merged at the next pathlevel release. :)

#9 Updated by Shyouhei Urabe over 2 years ago

  • Assignee changed from Hiroshi Nakamura to Shyouhei Urabe

#10 Updated by Charles Nutter over 1 year ago

Over a year and no progress on this one. If 1.8.7 is dead, this can be rejected...I'd just like to know.

#11 Updated by Motohiro KOSAKI about 1 year ago

  • Status changed from Assigned to Closed

1.8 is dead.

#12 Updated by Test Redmine 11 months ago

My 2 cents

Also available in: Atom PDF