[fix] Ensure that jruby-rack leaves ENV vars and Gem.path in consistent state

As noted at jruby/warbler#575 for warbler, it is similarly possible for jruby-rack to leave ENV['GEM_PATH'] and Gem.path in an inconsistent state after it updates them. This would make any later Gem.clear_paths or Bundler usage (driven from the ENV vars) unpredictable.

This happens because jruby-rack alters `ENV['GEM_PATH']``, in some cases but does not force Rubygems to recalculate Gem.paths. Normally this is fine, however if Gem.paths had already been used and cached prior to this mangling `Gem.path` will contain a "stale" value with respect to ENV. (e.g possibly missing the `GEM_HOME`/`Gem.default_dir` value, or retaining values from an old `GEM_PATH`. Clearing the paths is the only way to ensure things are consistent for later usage.

In particular this currently can happen on JRuby 9.4+ because often `Gem.path` is already initialized by the time we boot, due to the use of default gems such as `stringio`. Rather than assume a given state, it would be better to ensure it is consistent by clearing paths whenever we touch `ENV['GEM_PATH']`.

Author: chadlwilson

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 - 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
 
 ## 1.2.5
 

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

@@ -137,6 +137,8 @@ def adjust_gem_path
       else # 'jruby.rack.env.gem_path' "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

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
@@ -90,58 +99,85 @@
   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']
+    expect(@rack_context).to receive(:getInitParameter).with("jruby.rack.env.gem_path").and_return 'false'
+
     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 "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 +189,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 +251,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