Skip to content
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

require reloads file, not honoring partial path in LOADED_FEATURES #2477

Closed
camlow325 opened this issue Jan 18, 2015 · 3 comments
Closed

require reloads file, not honoring partial path in LOADED_FEATURES #2477

camlow325 opened this issue Jan 18, 2015 · 3 comments

Comments

@camlow325
Copy link
Contributor

This is related to a discussion in #1941.

In MRI Ruby 1.9, if a partial (relative?) path to a file is included in LOADED_FEATURES, an argument to 'require' matching that path will not result in the file being reloaded. In JRuby 1.7.18 and earlier, however, the same 'require' statement would cause the file to be reloaded.

For example, if "test/testmod.rb" were added to the LOADED_FEATURES, a subsequent "require 'test/testmod'" statement on MRI Ruby 1.9 would not reload the "test/testmod.rb" file whereas the same require statement would reload the "test/testmod.rb" file on JRuby 1.7.18.

Here are a couple of test Ruby files I used to observe this:

main.rb

puts "main - doing kernel load..."
Kernel.load("test/testmod.rb")

$LOADED_FEATURES << "test/testmod.rb"

puts "main - doing require..."
result = require 'test/testmod'

puts "main - require result: #{result}"

puts "main - testmod in $LOADED_FEATURES: "
$LOADED_FEATURES.each {|mod|
  if mod =~ /testmod/
    puts "- " + mod
  end
}

test/testmod.rb

module Testmod
  puts "testmod - sourcing..."
end

On MRI Ruby 1.9.3, I ran "ruby -I_base_dir_ main.rb" and saw the following output:

main - doing kernel load...
testmod - sourcing...
main - doing require...
main - require result: false
main - testmod in $LOADED_FEATURES:
- test/testmod.rb

On JRuby 1.7.18, I ran "jruby -I_base_dir_ main.rb" and saw the following output:

main - doing kernel load...
testmod - sourcing...
main - doing require...
testmod - sourcing...
main - require result: true
main - testmod in $LOADED_FEATURES:
- test/testmod.rb
- <base_dir>/test/testmod.rb

This issue affects Puppet's puppet_server project - https://github.com/puppetlabs/puppet-server. In Puppet Ruby code, there is some logic which manipulates LOADED_FEATURES directly in some cases where Puppet needs to manually manage the load process. For example, see https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/autoload.rb#L32-L37. The differences in MRI Ruby vs. JRuby interpretation of LOADED_FEATURES when a "require" is performed causes code to be reloaded unexpectedly. For cases where the loaded code is not idempotent - e.g., in various places where Puppet uses metaprogramming - the reload can lead to bad side effects like previously defined symbols later being undefined, etc. For example, see https://tickets.puppetlabs.com/browse/SERVER-40.

To more closely match MRI Ruby behavior, I'm thinking that the logic behind 'require' in JRuby would need to insert each entry into LOADED_FEATURES using the partial path loaded - e.g., 'test/testmod.rb' instead of '/full/path/to/test/testmod.rb' for a require of 'test/testmod' - and also match up the path supplied to each require against the partial path to see if the file has already been loaded - i.e., if a second "require 'test/testmod'" were encountered and "test/testmod.rb" were already in LOADED_FEATURES, the "test/testmod.rb" file would not be reloaded.

Hope this makes sense. Please let me know if there are more specific examples that would be helpful to demonstrate this issue.

@headius
Copy link
Member

headius commented Jan 26, 2015

This would probably be fixed if we did the same porting required for #2510. MRI has extensive logic during load to look for loaded features with no extension, with and without various load path entries, and so on. We have none of that logic in either 1.7 or 9k.

@enebo enebo modified the milestones: JRuby 1.7.19, JRuby 1.7.20 Jan 28, 2015
cprice404 added a commit to cprice404/puppet-lib-file_concat that referenced this issue Feb 27, 2015
Due to some issues in the way that Puppet loads code, and some
differences in how that works in JRuby vs MRI Ruby, doing a
`require` on the `puppet/type/file` code multiple times in a
single JRuby interpreter will cause problems.  Since this code
automatically gets loaded by Puppet before any custom
type/provider is loaded, the `require` statement is unnecessary.

This is related to the following Puppet Jira tickets:

https://tickets.puppetlabs.com/browse/SERVER-64
https://tickets.puppetlabs.com/browse/SERVER-40

and also to the following puppet-logstash PR:

voxpupuli/puppet-logstash#196

Hopefully there will be a more permanent fix in JRuby at some point:

jruby/jruby#2477

But in the meantime, this simple workaround makes `file_concat`
compatible with Puppet Server.
@headius
Copy link
Member

headius commented Apr 20, 2015

Note that #2510 has been fixed for 1.7.20 and 9k.pre2, but this is still an outstanding issue because we don't have the same heuristic for managing load path.

@enebo enebo modified the milestones: JRuby 1.7.25, JRuby 1.7.24 Jan 20, 2016
@enebo enebo modified the milestones: JRuby 1.7.26, JRuby 1.7.25 Apr 11, 2016
@enebo enebo modified the milestones: JRuby 1.7.26, JRuby 1.7.27 Aug 26, 2016
@headius
Copy link
Member

headius commented Mar 15, 2017

This is not going to get fixed in the 1.7 line. There's a separate issue for 9k to implement the path caching logic from MRI, which would include this partial-path logic.

@headius headius closed this as completed Mar 15, 2017
@headius headius modified the milestones: Won't Fix, JRuby 1.7.27 Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants