[fix] Remove the undocumented and unsafe jruby.rack.env.gem_path = false init parameter option

Changes the `false` value to behave the same way as empty, and not touch ENV['GEM_PATH'] or Gem.path at all. Previously this allowed you to alter Gem.path without touching the ENV['GEM_PATH']'. This was unsafe with Bundler 1.6+ (so for a long time), because many things may cause Gem.path to be recalculated from env, including bundler initialisations, restarts and later changes within an application's boot hooks. Furthermore the behaviour varied wildly depending on whether Rubygems was initialised prior to the Rack booter, or afterwards, even without bundler causing indeterminism and issues similar to jruby/warbler#575

I cannot find any reference to use of this value on GitHub, or discussion, and it would not work at all with modern JRuby/bundler so removing it, because it helps avoid further issues. Note that it was added in 2fd826b and then the default changed back in d3ee8f0 shortly after when issues were discovered, so given it's undocumented/unsupported, it is unlikely to be relied upon by anyone.

Author: chadlwilson

CHANGELOG.md

@@ -3,6 +3,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
+- Remove undocumented and unsafe jruby.rack.env.gem_path = false option (unusable on Bundler 1.6+)
 
 ## 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/booter.rb

@@ -104,55 +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/spec/ruby/jruby/rack/booter_spec.rb

@@ -98,16 +98,6 @@
     expect(booter.rack_env).to eq 'production'
   end
 
-  it "prepends gem_path to Gem.path (when configured to not mangle with ENV)" do
-    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
     ENV['GEM_PATH'] = "file:/home/gems#{File::PATH_SEPARATOR}/usr/local/gems"
     Gem.clear_paths
@@ -156,6 +146,20 @@
     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