Project

General

Profile

Actions

Bug #4181

closed

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

Added by headius (Charles Nutter) over 13 years ago. Updated almost 11 years ago.

Status:
Closed
Target version:
ruby -v:
Any Ruby 1.8 version
[ruby-core:33796]

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 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

Actions #1

Updated by headius (Charles Nutter) over 13 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

Actions #2

Updated by headius (Charles Nutter) about 13 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

Actions #3

Updated by naruse (Yui NARUSE) about 13 years ago

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

Actions #4

Updated by headius (Charles Nutter) about 13 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

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

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

Updated by nahi (Hiroshi Nakamura) almost 13 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
    
  •        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
    

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

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

Updated by nahi (Hiroshi Nakamura) over 12 years ago

  • Priority changed from 7 to Normal

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

Updated by shyouhei (Shyouhei Urabe) over 12 years ago

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

Updated by headius (Charles Nutter) over 11 years 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.

Updated by kosaki (Motohiro KOSAKI) about 11 years ago

  • Status changed from Assigned to Closed

1.8 is dead.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0