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

• Status: Closed
• Assignee: shyouhei (Shyouhei Urabe)
• Target version: Ruby 1.8.7
• ruby -v: Any Ruby 1.8 version

[headius (Charles Nutter)]

=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 singleton_killer.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 singleton_killer.rb:1:in to_proc' from singleton_killer.rb:21:in map'
from singleton_killer.rb:21
from singleton_killer.rb:5:in `loop'
from singleton_killer.rb:5

~/projects/jruby ➔ ruby -v singleton_killer.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 singleton_killer.rb:1:in to_proc' from singleton_killer.rb:21:in map'
from singleton_killer.rb:21
from singleton_killer.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

[headius (Charles Nutter)]

=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

[headius (Charles Nutter)]

=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

[naruse (Yui NARUSE)]

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

[headius (Charles Nutter)]

=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

[nahi (Hiroshi Nakamura)]

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

[nahi (Hiroshi Nakamura)]

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
  

•        remove_method :instance
  

•        def instance; @__instance__ end
  

•      # Check method existence to reduce redefinition warning.
  

•      unless self.respond_to?(: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
  

•        remove_method :instance
  

•        def instance; @__instance__ end
  

•      # See the comment for self.instance above.
  

•      unless self.respond_to?(:instance)
  

•        def self.instance; @__instance__ end
       end
     else
       @__instance__ = nil
  

[nahi (Hiroshi Nakamura)]

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

[nahi (Hiroshi Nakamura)]

• Priority changed from 7 to Normal

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

[shyouhei (Shyouhei Urabe)]

• Assignee changed from nahi (Hiroshi Nakamura) to shyouhei (Shyouhei Urabe)

[headius (Charles Nutter)]

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.

[kosaki (Motohiro KOSAKI)]

• Status changed from Assigned to Closed

1.8 is dead.

[testredmine (Test Redmine)]

My 2 cents