Bug #4181
closedBackport Ruby 1.9 singleton.rb, since 1.8's is not thread-safe
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
Updated by headius (Charles Nutter) almost 14 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
Updated by headius (Charles Nutter) almost 14 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
Updated by naruse (Yui NARUSE) almost 14 years ago
=begin
You mean we should copy 1.9's singleton.rb to 1.8 except "require 'thread'"?
=end
Updated by headius (Charles Nutter) almost 14 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) over 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) over 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) over 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 13 years ago
- Priority changed from 7 to Normal
Hope this would be merged at the next pathlevel release. :)
Updated by shyouhei (Shyouhei Urabe) about 13 years ago
- Assignee changed from nahi (Hiroshi Nakamura) to shyouhei (Shyouhei Urabe)
Updated by headius (Charles Nutter) almost 12 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) over 11 years ago
- Status changed from Assigned to Closed
1.8 is dead.
Updated by testredmine (Test Redmine) over 11 years ago
My 2 cents