Merge pull request #358 from chadlwilson/improve-gem-path-consistency

1.2.x: Ensure that Rubygems is not unnecessarily initialized and Gem.paths are left in consistent state

Author: chadlwilson

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 - Add missing block-only signature for debug logging
 - update (bundled) rack to 2.2.20
+- Ensure rack boot process leaves ENV['GEM_PATH'] and Gem.paths in a consistent state
+- Remove undocumented and unsafe jruby.rack.env.gem_path = false option (unusable on Bundler 1.6+)
+- Fix unintended Rubygems initialization too early in boot process with JRuby 9.4+
 
 ## 1.2.5
 

README.md

@@ -233,6 +233,12 @@ as context init parameters in web.xml or as VM-wide system properties.
 - `jruby.runtime.env.rubyopt`: This option is used for compatibility with the
   (deprecated) `jruby.rack.ignore.env` option since it cleared out the ENV after
   RUBYOPT has been processed, by setting it to true ENV['RUBYOPT'] will be kept.
+- `jruby.rack.env.gem_path`: If set to `true` (the default) jruby-rack will
+  ensure ENV['GEM_PATH'] is altered to include the `gem.path` above. If you set it to a
+  value, this value will be used as GEM_PATH, overriding the environment and
+  ignoring `gem.path` etc. By setting this option to en empty string the ENV['GEM_PATH'] will
+  not be modified by jruby-rack at all and will retain its original values implied by
+  the process environment and `jruby.runtime.env` setting.
 - `jruby.rack.logging`: Specify the logging device to use. Defaults to
   `servlet_context`. See below.
 - `jruby.rack.request.size.initial.bytes`: Initial size for request body memory

src/main/ruby/jruby/rack.rb

@@ -70,7 +70,6 @@ def context!; context || raise('no context available') end
   end
 end
 
-# TODO remove require 'jruby/rack/version' from jruby-rack in 1.2
 require 'jruby/rack/version' unless defined? JRuby::Rack::VERSION
 require 'jruby/rack/helpers'
 require 'jruby/rack/booter'

src/main/ruby/jruby/rack/booter.rb

@@ -104,53 +104,45 @@ def boot!
     protected
 
     def adjust_gem_path
-      gem_path = self.gem_path
+      desired_gem_path = self.gem_path
+
       case set_gem_path = env_gem_path
-      when true then
-        if env_path = ENV['GEM_PATH']
-          if gem_path.nil? || gem_path.empty?
-            return # keep ENV['GEM_PATH'] as is
-          elsif env_path != gem_path
-            separator = File::PATH_SEPARATOR
-            unless env_path.split(separator).include?(gem_path)
-              ENV['GEM_PATH'] = "#{gem_path}#{separator}#{env_path}"
-            end
-          end
-        else
-          ENV['GEM_PATH'] = gem_path
-        end
-      when false then
-        begin
-          require 'rubygems' unless defined? Gem.path
-        rescue LoadError
+      when true then # default behaviour
+        if (current_env_gem_path = ENV['GEM_PATH'])
+          # keep ENV['GEM_PATH'] as is if we have nothing to do
+          return if desired_gem_path.nil? || desired_gem_path.empty?
+          return if current_env_gem_path == desired_gem_path
+          return if current_env_gem_path.split(File::PATH_SEPARATOR).include?(desired_gem_path)
+
+          # need to prepend it
+          ENV['GEM_PATH'] = "#{desired_gem_path}#{File::PATH_SEPARATOR}#{current_env_gem_path}"
         else
-          return if gem_path.nil? || gem_path.empty?
-          Gem.path.unshift(gem_path) unless Gem.path.include?(gem_path)
+          ENV['GEM_PATH'] = desired_gem_path
         end
-        return false
-      when nil then # org.jruby.rack.RackLogger::DEBUG
-        if gem_path && ! gem_path.empty? &&
-          ( ! defined?(Gem.path) || ! Gem.path.include?(gem_path) )
-          @rack_context.log("Gem.path won't be updated although seems configured: #{gem_path}")
+      when nil then
+        if desired_gem_path && !desired_gem_path.empty? && (!defined?(Gem.path) || !Gem.path.include?(desired_gem_path) )
+          @rack_context.log("Gem.path won't be updated although seems configured: #{desired_gem_path}")
         end
-        return nil
-      else # 'jruby.rack.env.gem_path' "forced" to an explicit value
+        return nil # do nothing to ENV['GEM_PATH']
+      else # "forced" to an explicit value
         ENV['GEM_PATH'] = set_gem_path
       end
+      # Whenever we touch ENV['GEM_PATH`], ensure we clear any cached paths. All other cases should exit early.
+      Gem.clear_paths if defined?(Gem.clear_paths)
     end
 
-    # @return whether to update Gem.path and/or the environment GEM_PATH
-    # - true (default) forces ENV['GEM_PATH'] to be updated due compatibility
-    #   Bundler 1.6 fails to revolve gems correctly when Gem.path is updated
-    #   instead of the ENV['GEM_PATH'] environment variable
-    # - false disables ENV['GEM_PATH'] mangling for good (updates Gem.path)
+    # @return whether to update the environment GEM_PATH
+    # - true (default) forces ENV['GEM_PATH'] to be updated to include the `gem.path` above.
+    #   If you set it to a non-empty value, GEM_PATH will be forced to an explicit value,
+    #   overriding the environment and ignoring `gem.path`, `gem.home` etc.
     #
-    # - if not specified Gem.path will be updated based on setting
+    # - By setting this option to an empty string the ENV['GEM_PATH'] should not be modified
+    #   at all and will retain its original values implied by the process environment and
+    #  `jruby.runtime.env` setting.
     def env_gem_path
       gem_path = @rack_context.getInitParameter('jruby.rack.env.gem_path')
       return true if gem_path.nil? || gem_path.to_s == 'true'
-      return false if gem_path.to_s == 'false'
-      return nil if gem_path.empty? # set to an empty disables mangling
+      return nil if gem_path.empty? || gem_path.to_s == 'false' # treat false as "don't touch either ENV or Gem.path"
       gem_path
     end
     private :env_gem_path

src/main/ruby/jruby/rack/capture.rb

@@ -5,13 +5,11 @@
 # See the file LICENSE.txt for details.
 #++
 
-require 'stringio'
-
 module JRuby::Rack
   module Capture
     module Base
       def output
-        @output ||= begin; StringIO.new; end
+        @output ||= begin; require 'stringio' unless defined?(StringIO); StringIO.new; end
       end
 
       def capture
@@ -34,6 +32,7 @@ def store
     module Exception
       def output
         @output ||= begin
+          require 'stringio' unless defined?(StringIO)
           io = StringIO.new
           io.puts "An exception happened during JRuby-Rack startup", self.to_s
           io

src/spec/ruby/jruby/rack/booter_spec.rb

@@ -17,15 +17,24 @@
   after(:all) { JRuby::Rack.context = nil }
 
   before do
-    @rack_env = ENV['RACK_ENV']
-    @gem_path = Gem.path.to_a
-    @env_gem_path = ENV['GEM_PATH']
+    # start clean, in case another test was messing with paths
+    Gem.clear_paths
+    @original_rack_env = ENV['RACK_ENV']
+    @original_gem_path = Gem.path.to_a
+    @original_env_gem_path = ENV['GEM_PATH']
   end
 
   after do
-    @rack_env.nil? ? ENV.delete('RACK_ENV') : ENV['RACK_ENV'] = @rack_env
-    Gem.path.replace(@gem_path)
-    @env_gem_path.nil? ? ENV.delete('GEM_PATH') : ENV['GEM_PATH'] = @env_gem_path
+    # Ensure everything is reset how it was
+    @original_rack_env.nil? ? ENV.delete('RACK_ENV') : ENV['RACK_ENV'] = @original_rack_env
+    @original_env_gem_path.nil? ? ENV.delete('GEM_PATH') : ENV['GEM_PATH'] = @original_env_gem_path
+    Gem.clear_paths
+    Gem.path.replace(@original_gem_path)
+
+    aggregate_failures("expected Gem.path to be restored after test") do
+      expect(ENV['GEM_PATH']).to eq @original_env_gem_path
+      expect(Gem.path).to eql @original_gem_path
+    end
   end
 
   it "should determine the public html root from the 'public.root' init parameter" do
@@ -89,59 +98,90 @@
     expect(booter.rack_env).to eq 'production'
   end
 
-  it "prepends gem_path to Gem.path (when configured to not mangle with ENV)" do
-    expect(@rack_context).to receive(:getInitParameter).with("jruby.rack.env.gem_path").and_return 'false'
-    Gem.path.replace ['/opt/gems']
-    booter.gem_path = "wsjar:file:/opt/deploy/sample.war!/WEB-INF/gems"
-    booter.boot!
-
-    expect(Gem.path).to eql ['wsjar:file:/opt/deploy/sample.war!/WEB-INF/gems', '/opt/gems']
-  end
-
   it "prepends gem_path to Gem.path if not already present" do
-    Gem.path.replace ["file:/home/gems", "/usr/local/gems"]
+    ENV['GEM_PATH'] = "file:/home/gems#{File::PATH_SEPARATOR}/usr/local/gems"
+    Gem.clear_paths
+
     booter.gem_path = '/usr/local/gems'
     booter.boot!
 
-    expect(Gem.path).to eql ["file:/home/gems", "/usr/local/gems"]
+    expect(Gem.path).to start_with ["file:/home/gems", "/usr/local/gems"]
+    expect(ENV['GEM_PATH']).to eq "file:/home/gems#{File::PATH_SEPARATOR}/usr/local/gems"
   end
 
   it "does not change Gem.path if gem_path empty" do
-    Gem.path.replace ['/opt/gems']
+    ENV['GEM_PATH'] = '/opt/gems'
+    Gem.clear_paths
+
     booter.gem_path = ""
     booter.boot!
 
-    expect(Gem.path).to eql ['/opt/gems']
+    expect(Gem.path).to start_with ['/opt/gems']
+    expect(ENV['GEM_PATH']).to eq '/opt/gems'
   end
 
   it "prepends gem_path to ENV['GEM_PATH'] if jruby.rack.gem_path set to true" do
     expect(@rack_context).to receive(:getInitParameter).with("jruby.rack.env.gem_path").and_return 'true'
     ENV['GEM_PATH'] = '/opt/gems'
+    Gem.clear_paths
     expect(@rack_context).to receive(:getRealPath).with("/WEB-INF").and_return "/opt/deploy/sample.war!/WEB-INF"
     expect(@rack_context).to receive(:getRealPath).with("/WEB-INF/gems").and_return "/opt/deploy/sample.war!/WEB-INF/gems"
 
     booter.boot!
 
     expect(ENV['GEM_PATH']).to eq "/opt/deploy/sample.war!/WEB-INF/gems#{File::PATH_SEPARATOR}/opt/gems"
+    expect(Gem.path).to start_with ["/opt/deploy/sample.war!/WEB-INF/gems", "/opt/gems"]
   end
 
   it "does not prepend gem_path to ENV['GEM_PATH'] if jruby.rack.gem_path set not set" do
     expect(@rack_context).to receive(:getInitParameter).with("jruby.rack.env.gem_path").and_return ''
     ENV['GEM_PATH'] = '/opt/gems'
+    Gem.clear_paths
     expect(@rack_context).to receive(:getRealPath).with("/WEB-INF").and_return "/opt/deploy/sample.war!/WEB-INF"
     expect(@rack_context).to receive(:getRealPath).with("/WEB-INF/gems").and_return "/opt/deploy/sample.war!/WEB-INF/gems"
 
     booter.boot!
 
     expect(ENV['GEM_PATH']).to eq "/opt/gems"
+    expect(Gem.path).to start_with ["/opt/gems"]
+  end
+
+  it "does not prepend gem_path to ENV['GEM_PATH'] if jruby.rack.gem_path set to false" do
+    ENV['GEM_PATH'] = '/opt/gems'
+    Gem.clear_paths
+
+    expect(@rack_context).to receive(:getInitParameter).with("jruby.rack.env.gem_path").and_return 'false'
+    expect(@rack_context).to receive(:getRealPath).with("/WEB-INF").and_return "/opt/deploy/sample.war!/WEB-INF"
+    expect(@rack_context).to receive(:getRealPath).with("/WEB-INF/gems").and_return "/opt/deploy/sample.war!/WEB-INF/gems"
+
+    booter.boot!
+
+    expect(ENV['GEM_PATH']).to eq "/opt/gems"
+    expect(Gem.path).to start_with ["/opt/gems"]
   end
 
   it "prepends gem_path to ENV['GEM_PATH'] if not already present" do
     ENV['GEM_PATH'] = "/home/gems#{File::PATH_SEPARATOR}/usr/local/gems"
+    Gem.clear_paths
     booter.gem_path = '/usr/local/gems'
     booter.boot!
 
     expect(ENV['GEM_PATH']).to eq "/home/gems#{File::PATH_SEPARATOR}/usr/local/gems"
+    expect(Gem.path).to start_with ["/home/gems", "/usr/local/gems"]
+  end
+
+  it "keeps ENV['GEM_PATH'] when gem_path is nil" do
+    ENV['GEM_PATH'] = '/usr/local/gems'
+    Gem.clear_paths
+
+    booter.layout = layout = double('layout')
+    allow(layout).to receive(:app_path).and_return '.'
+    allow(layout).to receive(:public_path).and_return nil
+    expect(layout).to receive(:gem_path).and_return nil
+    booter.boot!
+
+    expect(ENV['GEM_PATH']).to eq "/usr/local/gems"
+    expect(Gem.path).to start_with ["/usr/local/gems"]
   end
 
   it "sets ENV['GEM_PATH'] to the value of gem_path if ENV['GEM_PATH'] is not present" do
@@ -153,6 +193,7 @@
     booter.boot!
 
     expect(ENV['GEM_PATH']).to eq "/blah/gems"
+    expect(Gem.path).to start_with ["/blah/gems"]
   end
 
   it "creates a logger that writes messages to the servlet context (by default)" do
@@ -214,11 +255,12 @@
     #    at RUBY.boot!(classpath:/jruby/rack/booter.rb:105)
     #    at RUBY.(root)(classpath:/jruby/rack/boot/rack.rb:10)
     app_dir = "#{File.absolute_path Dir.pwd}/sample.war!/WEB-INF"
-    allow(File).to receive(:directory?).with(app_dir).and_return true
     allow(booter).to receive(:layout).and_return layout = double('layout')
     allow(layout).to receive(:app_path).and_return app_dir
     allow(layout).to receive(:gem_path)
     allow(layout).to receive(:public_path)
+    allow(File).to receive(:directory?).and_wrap_original { |m, *args| m.call(*args) }
+    expect(File).to receive(:directory?).with(app_dir).and_return true
 
     booter.boot! # expect to_not raise_error
   end

src/spec/ruby/jruby/rack/rails_booter_spec.rb

@@ -30,13 +30,13 @@
   end
 
   before do
-    @rails_env = ENV['RAILS_ENV']
-    @rack_env = ENV['RACK_ENV']
+    @original_rails_env = ENV['RAILS_ENV']
+    @original_rack_env = ENV['RACK_ENV']
   end
 
   after do
-    @rails_env.nil? ? ENV.delete('RAILS_ENV') : ENV['RAILS_ENV'] = @rails_env
-    @rack_env.nil? ? ENV.delete('RACK_ENV') : ENV['RACK_ENV'] = @rack_env
+    @original_rails_env.nil? ? ENV.delete('RAILS_ENV') : ENV['RAILS_ENV'] = @original_rails_env
+    @original_rack_env.nil? ? ENV.delete('RACK_ENV') : ENV['RACK_ENV'] = @original_rack_env
   end
 
   it "should default rails path to /WEB-INF" do