New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Module#const_get may return incomplete class #2945

Closed
yousuketto opened this Issue May 15, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@yousuketto
Contributor

yousuketto commented May 15, 2015

I've encountered when executing ActiveResource::Base.find_or_create_resource_for.
https://github.com/rails/activeresource/blob/v4.0.0/lib/active_resource/base.rb#L1501-L1520
name method of incomplete class return nil.

simple reproduction

# example.rb
module Xxx
  class << self
    def find_or_create(klass_name)
      self.const_get(klass_name)
    rescue NameError => e
      self.const_set(klass_name, Class.new)
    end
  end
end

ths = []
200.times do |name|
  100.times do |index|
    t = Thread.new(index.to_s, name.to_s) do |i, n|
      klass_name = "Yyy#{n}"
      yyy = Xxx.find_or_create(klass_name)
      unless yyy.name
        print "hit!! #{klass_name}.name == nil\n"
      end
    end
    ths << t
  end
end
ths.each(&:join)
$ jruby -v
jruby 1.7.20 (1.9.3p551) 2015-05-04 3086e6a on Java HotSpot(TM) 64-Bit Server VM 1.8.0_45-b14 +jit [darwin-x86_64]
$ jruby example.rb 
example.rb:6 warning: already initialized constant Yyy6
example.rb:6 warning: already initialized constant Yyy6
example.rb:6 warning: already initialized constant Yyy10
example.rb:6 warning: already initialized constant Yyy11
example.rb:6 warning: already initialized constant Yyy11
...
example.rb:6 warning: already initialized constant Yyy84
example.rb:6 warning: already initialized constant Yyy84
hit!! Yyy85.name == nil
example.rb:6 warning: already initialized constant Yyy92
example.rb:6 warning: already initialized constant Yyy93
...
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares May 15, 2015

Member

this is all fine on JRuby's end - you should not expect your code to not print warnings since it's not thread-safe ... JRuby simply can not "know" that :

    def find_or_create(klass_name)
      self.const_get(klass_name)
    rescue NameError => e
      self.const_set(klass_name, Class.new)
    end

is intended to be somehow "atomic" ... would report this on Rails as their code assumes MRI (gil)

Member

kares commented May 15, 2015

this is all fine on JRuby's end - you should not expect your code to not print warnings since it's not thread-safe ... JRuby simply can not "know" that :

    def find_or_create(klass_name)
      self.const_get(klass_name)
    rescue NameError => e
      self.const_set(klass_name, Class.new)
    end

is intended to be somehow "atomic" ... would report this on Rails as their code assumes MRI (gil)

@kares kares added this to the Invalid or Duplicate milestone May 15, 2015

@yousuketto

This comment has been minimized.

Show comment
Hide comment
@yousuketto

yousuketto May 15, 2015

Contributor

Sorry, I'm weak in English.

I don't think that warning message is a problem.

Yyy85 was executed name method, and returned nil.

hit!! Yyy85.name == nil

Therefore, Yyy85 was incomplete state.
It is a problem that I could get this class of incomplete state.

I think cause that's the order of processing.

  1. Class object was registered in the constant table.
  2. Constant name was registred.
Contributor

yousuketto commented May 15, 2015

Sorry, I'm weak in English.

I don't think that warning message is a problem.

Yyy85 was executed name method, and returned nil.

hit!! Yyy85.name == nil

Therefore, Yyy85 was incomplete state.
It is a problem that I could get this class of incomplete state.

I think cause that's the order of processing.

  1. Class object was registered in the constant table.
  2. Constant name was registred.
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares May 15, 2015

Member

oh sorry, I missed that completely .. yes the name is an issue (I've actually run into that as well in JI)

Member

kares commented May 15, 2015

oh sorry, I missed that completely .. yes the name is an issue (I've actually run into that as well in JI)

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 28, 2015

Member

This is not fixable in JRuby.

MRI determines name in a very peculiar way: when it has not yet been set for a given class, it walks every class in the system accessible via Object and uses that to build the name.

This has some severe performance implications that most people don't know about: https://bugs.ruby-lang.org/issues/11119

Needless to say, we won't be implementing #name that way. Instead, we use the usually-ok logic of assigning name when the class is first set to a constant. This behavior you see in Rails is exploting a very strange edge case, and it won't work under JRuby or Rubinius.

Member

headius commented May 28, 2015

This is not fixable in JRuby.

MRI determines name in a very peculiar way: when it has not yet been set for a given class, it walks every class in the system accessible via Object and uses that to build the name.

This has some severe performance implications that most people don't know about: https://bugs.ruby-lang.org/issues/11119

Needless to say, we won't be implementing #name that way. Instead, we use the usually-ok logic of assigning name when the class is first set to a constant. This behavior you see in Rails is exploting a very strange edge case, and it won't work under JRuby or Rubinius.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 28, 2015

Member

We may be able to mitigate this by setting the name before we assign the constant...I think that would be reasonable and be a closer approximation of MRI's behavior.

Member

headius commented May 28, 2015

We may be able to mitigate this by setting the name before we assign the constant...I think that would be reasonable and be a closer approximation of MRI's behavior.

@enebo enebo modified the milestones: JRuby 1.7.21, JRuby 1.7.22 Jul 7, 2015

@kares kares modified the milestones: JRuby 1.7.23, JRuby 1.7.22 Aug 20, 2015

@enebo enebo modified the milestones: JRuby 1.7.23, JRuby 1.7.24 Nov 24, 2015

kares added a commit to kares/jruby that referenced this issue Jan 2, 2016

kares added a commit to kares/jruby that referenced this issue Jan 2, 2016

Merge branch 'jruby-1_7' into master
* jruby-1_7: (46 commits)
  calculate module name before storing in parent's constant hierarchy (resolves #2945)
  remove double imports in RubyModule
  remove (unnecessary) global variable in spec_helper
  Symbol#to_proc procs' #parameters always return [[:rest]].
  remove com.bounday:high-scale-lib dependency (extracted necessary classes from it)
  AtomicReferenceFieldUpdater forces us to have volatile fields
  use the (internal) backported NonBlockingHashMapLong class instead of high_scale's
  make Unsafe usage in NonBlockingHashMapLong (ported from high-scale lib) optional
  add (and re-arrange) AbstractEntry class - NonBlockingHashMapLong now compilable
  NonBlockingHashMapLong - add in required Counter class and comment out prints
  NonBlockingHashMapLong copy-pasted from org.cliffc.high_scale_lib package
  [build] remove the rake-plugin from release
  [build] use newer versions for jruby-rake-plugin [skip ci]
  handle "junk" tail in BigDecimal parsing like MRI does (fixes #3527)
  Update FileStat to return nil for methods MRI does not implement (nor do we)
  no singleton_class in 1.8.7
  sorting by name is not reliable as classes are all anonymous - sort by object_id instead
  each_object(cls.singleton_class) should not walk special classes.
  Bump for next dev cycle
  Whoops forgot to change VERSION
  ...

@kares kares closed this Jan 2, 2016

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