Merge #6728

6728: Fallback to sequentially fetching specs on 429s r=indirect a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem is that sometimes `bundler` is unable to resolve certain gemfiles. Specifically, sometimes it does not respect the `required_ruby_version` setting. This causes some people to assume that `bundler` will always try to install the latest version of any dependency, regardless of the ruby version being run, because as a matter of fact, this feature sometimes just doesn't work. See for example the discussion at https://github.com/rspec/rspec/issues/25.

The problem was consistently reproducible until a few minutes ago with the following `Gemfile` under `ruby 2.3.7`

```ruby
source "https://rubygems.org"
ruby "2.3.7"
gem "berkshelf", "= 6.3.1"
```

```
$ docker run -it --rm --volume $(pwd):/app ruby:2.3.7 sh -c "cd /app && rm -f Gemfile.lock && bundle install"
Fetching gem metadata from https://rubygems.org/.............
Fetching gem metadata from https://rubygems.org/..
Resolving dependencies....
Fetching public_suffix 3.0.3
Installing public_suffix 3.0.3
Fetching addressable 2.5.2
Installing addressable 2.5.2
Fetching buff-extensions 2.0.0
Installing buff-extensions 2.0.0
Fetching hashie 3.6.0
Installing hashie 3.6.0
Fetching varia_model 0.6.0
Installing varia_model 0.6.0
Fetching buff-config 2.0.0
Installing buff-config 2.0.0
Using bundler 1.16.5
Fetching fuzzyurl 0.9.0
Installing fuzzyurl 0.9.0
Fetching tomlrb 1.2.7
Installing tomlrb 1.2.7
Fetching mixlib-config 2.2.13
Installing mixlib-config 2.2.13
Fetching mixlib-shellout 2.4.0
Installing mixlib-shellout 2.4.0
Fetching chef-config 14.5.33
Installing chef-config 14.5.33
Fetching libyajl2 1.2.0
Installing libyajl2 1.2.0 with native extensions
Fetching ffi-yajl 2.3.1
Installing ffi-yajl 2.3.1 with native extensions
Fetching mixlib-log 2.0.4
Installing mixlib-log 2.0.4
Fetching rack 2.0.5
Installing rack 2.0.5
Fetching uuidtools 2.1.5
Installing uuidtools 2.1.5
Fetching chef-zero 14.0.6
Installing chef-zero 14.0.6
Gem::RuntimeRequirementNotMetError: chef-zero requires Ruby version >= 2.4.0. The current ruby version is 2.3.0.
An error occurred while installing chef-zero (14.0.6), and Bundler cannot continue.
Make sure that `gem install chef-zero -v '14.0.6' --source 'https://rubygems.org/'` succeeds before bundling.

In Gemfile:
  berkshelf was resolved to 6.3.1, which depends on
    chef was resolved to 14.5.33, which depends on
      chef-zero
```

Funny enough, I can no longer reproduce it at the moment, I guess it depends on the specific load conditions of the rubygems.org servers?

### What was your diagnosis of the problem?

My diagnosis was that sometimes our resolution falls back to the old dependency API that didn't implement the `required_ruby_version` setting. In particular, this happens because the new API returns `Net::HTTPTooManyRequests`, so `bundler` gives up and defaults to the old API.

### What is your fix for the problem, implemented in this PR?

My fix is to, instead of directly fall back to the old API when rate limiting happens, try first to fetch the dependencies sequentially instead of in parallel still from the new API, so that rate limit does not affect us.

### Why did you choose this fix out of the possible options?

I chose this fix because it was the only idea that came up. As a matter of fact, #6471 and #6639 were closed because there was nothing we could do, so it seems like it's the only idea so far :)

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

6 files changed, 118 insertions, 16 deletions

diff --git a/lib/bundler/compact_index_client.rb b/lib/bundler/compact_index_client.rb
index 6c241ca07a..2f713041c8 100644
--- a/lib/bundler/compact_index_client.rb
+++ b/lib/bundler/compact_index_client.rb
@@ -18,11 +18,6 @@ module Bundler
 
     attr_reader :directory
 
-    # @return [Lambda] A lambda that takes an array of inputs and a block, and
-    #         maps the inputs with the block in parallel.
-    #
-    attr_accessor :in_parallel
-
     def initialize(directory, fetcher)
       @directory = Pathname.new(directory)
       @updater = Updater.new(fetcher)
@@ -31,7 +26,28 @@ module Bundler
       @info_checksums_by_name = {}
       @parsed_checksums = false
       @mutex = Mutex.new
-      @in_parallel = lambda do |inputs, &blk|
+    end
+
+    def execution_mode=(block)
+      Bundler::CompactIndexClient.debug { "execution_mode=" }
+      @endpoints = Set.new
+
+      @execution_mode = block
+    end
+
+    # @return [Lambda] A lambda that takes an array of inputs and a block, and
+    #         maps the inputs with the block in parallel.
+    #
+    def execution_mode
+      @execution_mode || sequentially
+    end
+
+    def sequential_execution_mode!
+      self.execution_mode = sequentially
+    end
+
+    def sequentially
+      @sequentially ||= lambda do |inputs, &blk|
         inputs.map(&blk)
       end
     end
@@ -51,7 +67,7 @@ module Bundler
 
     def dependencies(names)
       Bundler::CompactIndexClient.debug { "dependencies(#{names})" }
-      in_parallel.call(names) do |name|
+      execution_mode.call(names) do |name|
         update_info(name)
         @cache.dependencies(name).map {|d| d.unshift(name) }
       end.flatten(1)
diff --git a/lib/bundler/fetcher.rb b/lib/bundler/fetcher.rb
index 7df42adc34..272aac6882 100644
--- a/lib/bundler/fetcher.rb
+++ b/lib/bundler/fetcher.rb
@@ -15,6 +15,8 @@ module Bundler
 
     # This error is raised when it looks like the network is down
     class NetworkDownError < HTTPError; end
+    # This error is raised if we should rate limit our requests to the API
+    class TooManyRequestsError < HTTPError; end
     # This error is raised if the API returns a 413 (only printed in verbose)
     class FallbackError < HTTPError; end
     # This is the error raised if OpenSSL fails the cert verification
diff --git a/lib/bundler/fetcher/compact_index.rb b/lib/bundler/fetcher/compact_index.rb
index cfc74d642c..a117af72fa 100644
--- a/lib/bundler/fetcher/compact_index.rb
+++ b/lib/bundler/fetcher/compact_index.rb
@@ -39,7 +39,13 @@ module Bundler
         until remaining_gems.empty?
           log_specs "Looking up gems #{remaining_gems.inspect}"
 
-          deps = compact_index_client.dependencies(remaining_gems)
+          deps = begin
+                   parallel_compact_index_client.dependencies(remaining_gems)
+                 rescue TooManyRequestsError
+                   @bundle_worker.stop if @bundle_worker
+                   @bundle_worker = nil # reset it.  Not sure if necessary
+                   serial_compact_index_client.dependencies(remaining_gems)
+                 end
           next_gems = deps.map {|d| d[3].map(&:first).flatten(1) }.flatten(1).uniq
           deps.each {|dep| gem_info << dep }
           complete_gems.concat(deps.map(&:first)).uniq!
@@ -80,18 +86,26 @@ module Bundler
     private
 
       def compact_index_client
-        @compact_index_client ||= begin
+        @compact_index_client ||=
           SharedHelpers.filesystem_access(cache_path) do
             CompactIndexClient.new(cache_path, client_fetcher)
-          end.tap do |client|
-            client.in_parallel = lambda do |inputs, &blk|
-              func = lambda {|object, _index| blk.call(object) }
-              worker = bundle_worker(func)
-              inputs.each {|input| worker.enq(input) }
-              inputs.map { worker.deq }
-            end
           end
+      end
+
+      def parallel_compact_index_client
+        compact_index_client.execution_mode = lambda do |inputs, &blk|
+          func = lambda {|object, _index| blk.call(object) }
+          worker = bundle_worker(func)
+          inputs.each {|input| worker.enq(input) }
+          inputs.map { worker.deq }
         end
+
+        compact_index_client
+      end
+
+      def serial_compact_index_client
+        compact_index_client.sequential_execution_mode!
+        compact_index_client
       end
 
       def bundle_worker(func = nil)
diff --git a/lib/bundler/fetcher/downloader.rb b/lib/bundler/fetcher/downloader.rb
index 87ad4140fd..2aeb9962c4 100644
--- a/lib/bundler/fetcher/downloader.rb
+++ b/lib/bundler/fetcher/downloader.rb
@@ -34,6 +34,8 @@ module Bundler
           fetch(uri, new_headers)
         when Net::HTTPRequestEntityTooLarge
           raise FallbackError, response.body
+        when Net::HTTPTooManyRequests
+          raise TooManyRequestsError, response.body
         when Net::HTTPUnauthorized
           raise AuthenticationRequiredError, uri.host
         when Net::HTTPNotFound
diff --git a/spec/install/gems/resolving_spec.rb b/spec/install/gems/resolving_spec.rb
index 0be7a56606..cf3aaa719e 100644
--- a/spec/install/gems/resolving_spec.rb
+++ b/spec/install/gems/resolving_spec.rb
@@ -117,6 +117,26 @@ RSpec.describe "bundle install with install-time dependencies" do
         expect(out).to_not include("rack-9001.0.0 requires ruby version > 9000")
         expect(the_bundle).to include_gems("rack 1.2")
       end
+
+      it "installs the older version under rate limiting conditions" do
+        build_repo4 do
+          build_gem "rack", "9001.0.0" do |s|
+            s.required_ruby_version = "> 9000"
+          end
+          build_gem "rack", "1.2"
+          build_gem "foo1", "1.0"
+        end
+
+        install_gemfile <<-G, :artifice => "compact_index_rate_limited", :env => { "BUNDLER_SPEC_GEM_REPO" => gem_repo4 }
+          ruby "#{RUBY_VERSION}"
+          source "http://localgemserver.test/"
+          gem 'rack'
+          gem 'foo1'
+        G
+
+        expect(out).to_not include("rack-9001.0.0 requires ruby version > 9000")
+        expect(the_bundle).to include_gems("rack 1.2")
+      end
     end
 
     context "allows no gems" do
diff --git a/spec/support/artifice/compact_index_rate_limited.rb b/spec/support/artifice/compact_index_rate_limited.rb
new file mode 100644
index 0000000000..d8f4fc941c
--- /dev/null
+++ b/spec/support/artifice/compact_index_rate_limited.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require File.expand_path("../compact_index", __FILE__)
+
+Artifice.deactivate
+
+class CompactIndexRateLimited < CompactIndexAPI
+  class RequestCounter
+    def self.queue
+      @queue ||= Queue.new
+    end
+
+    def self.size
+      @queue.size
+    end
+
+    def self.enq(name)
+      @queue.enq(name)
+    end
+
+    def self.deq
+      @queue.deq
+    end
+  end
+
+  configure do
+    RequestCounter.queue
+  end
+
+  get "/info/:name" do
+    RequestCounter.enq(params[:name])
+
+    begin
+      if RequestCounter.size == 1
+        etag_response do
+          gem = gems.find {|g| g.name == params[:name] }
+          CompactIndex.info(gem ? gem.versions : [])
+        end
+      else
+        status 429
+      end
+    ensure
+      RequestCounter.deq
+    end
+  end
+end
+
+Artifice.activate_with(CompactIndexRateLimited)