Auto merge of #6022 - bundler:seg-fix-fetcher-regression, r=colby-swandale

Avoid making unnecessary network requests fetching specs

The problem was installing `source "https://rubygems.org"; gem "rack"` could cause bundler to make requests for _hundreds_ of specs, drastically slowing down installation. This was a regression in 1.16 caused by the new source pinning logic.

My diagnosis was the "double checking" step of creating the definition's index was accidentally saying Bundler needed to download specs for _every gem installed_, along with their dependencies _for every version in existence_.

My fix narrows down the set of names that need to be double-checked for to (a) only those that could possibly be needed for resolution (b) and only those specs that could possibly come from many sources (i.e. are "unpinned")

I chose this fix because it takes that `rack` gemfile back down to __2__ requests: `/versions` and `/info/rack`, as it should be, while maintaining proper searching for back-deps.

(cherry picked from commit c1668a569244145790c627a08aa0f0f4720031ad)

11 files changed, 122 insertions, 53 deletions

diff --git a/bin/bundle1 b/bin/bundle1
new file mode 100755
index 0000000000..b5df992269
--- /dev/null
+++ b/bin/bundle1
@@ -0,0 +1,8 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+module Bundler
+  VERSION = "1.98".freeze
+end
+
+load File.expand_path("../bundle", __FILE__)
diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb
index 1385ff2a94..486ef85a64 100644
--- a/lib/bundler/definition.rb
+++ b/lib/bundler/definition.rb
@@ -265,9 +265,8 @@ module Bundler
         dependency_names = @dependencies.map(&:name)
 
         sources.all_sources.each do |source|
-          source.dependency_names = dependency_names.dup
+          source.dependency_names = dependency_names - pinned_spec_names(source)
           idx.add_source source.specs
-          dependency_names -= pinned_spec_names(source.specs)
           dependency_names.concat(source.unmet_deps).uniq!
         end
 
@@ -281,21 +280,25 @@ module Bundler
     # of Foo specifically depends on a version of Bar that is only found in source B. This ensures that for
     # each spec we found, we add all possible versions from all sources to the index.
     def double_check_for_index(idx, dependency_names)
+      pinned_names = pinned_spec_names
       loop do
         idxcount = idx.size
+
+        names = :names # do this so we only have to traverse to get dependency_names from the index once
+        unmet_dependency_names = lambda do
+          return names unless names == :names
+          new_names = sources.all_sources.map(&:dependency_names_to_double_check)
+          return names = nil if new_names.compact!
+          names = new_names.flatten(1).concat(dependency_names)
+          names.uniq!
+          names -= pinned_names
+          names
+        end
+
         sources.all_sources.each do |source|
-          names = :names # do this so we only have to traverse to get dependency_names from the index once
-          unmet_dependency_names = proc do
-            break names unless names == :names
-            names = if idx.size > Source::Rubygems::API_REQUEST_LIMIT
-              new_names = idx.dependency_names_if_available
-              new_names && dependency_names.+(new_names).uniq
-            else
-              dependency_names.+(idx.dependency_names).uniq
-            end
-          end
           source.double_check_for(unmet_dependency_names, :override_dupes)
         end
+
         break if idxcount == idx.size
       end
     end
@@ -916,18 +919,15 @@ module Bundler
       source_requirements
     end
 
-    def pinned_spec_names(specs)
-      names = []
-      specs.each do |s|
-        # TODO: when two sources without blocks is an error, we can change
-        # this check to !s.source.is_a?(Source::LocalRubygems). For now,
-        # we need to ask every RubyGems for every gem name.
-        if s.source.is_a?(Source::Git) || s.source.is_a?(Source::Path)
-          names << s.name
-        end
+    def pinned_spec_names(skip = nil)
+      pinned_names = []
+      default = Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? && sources.default_source
+      @dependencies.each do |dep|
+        next unless dep_source = dep.source || default
+        next if dep_source == skip
+        pinned_names << dep.name
       end
-      names.uniq!
-      names
+      pinned_names
     end
 
     def requested_groups
diff --git a/lib/bundler/fetcher/dependency.rb b/lib/bundler/fetcher/dependency.rb
index 741b81acac..1430d1ebeb 100644
--- a/lib/bundler/fetcher/dependency.rb
+++ b/lib/bundler/fetcher/dependency.rb
@@ -7,7 +7,7 @@ module Bundler
   class Fetcher
     class Dependency < Base
       def available?
-        fetch_uri.scheme != "file" && downloader.fetch(dependency_api_uri)
+        @available ||= fetch_uri.scheme != "file" && downloader.fetch(dependency_api_uri)
       rescue NetworkDownError => e
         raise HTTPError, e.message
       rescue AuthenticationRequiredError
diff --git a/lib/bundler/index.rb b/lib/bundler/index.rb
index 8d496f1e2b..9166a92738 100644
--- a/lib/bundler/index.rb
+++ b/lib/bundler/index.rb
@@ -115,6 +115,12 @@ module Bundler
       self
     end
 
+    def spec_names
+      names = specs.keys + sources.map(&:spec_names)
+      names.uniq!
+      names
+    end
+
     # returns a list of the dependencies
     def unmet_dependency_names
       dependency_names.select do |name|
@@ -133,19 +139,6 @@ module Bundler
       names.uniq
     end
 
-    def dependency_names_if_available
-      reduce([]) do |names, spec|
-        case spec
-        when EndpointSpecification, Gem::Specification, LazySpecification, StubSpecification
-          names.concat(spec.dependencies)
-        when RemoteSpecification # from the full index
-          return nil
-        else
-          raise "unhandled spec type in #dependency_names_if_available (#{spec.inspect})"
-        end
-      end.tap {|n| n && n.map!(&:name) }
-    end
-
     def use(other, override_dupes = false)
       return unless other
       other.each do |s|
diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb
index 348b64ff4e..c25c853f11 100644
--- a/lib/bundler/resolver.rb
+++ b/lib/bundler/resolver.rb
@@ -38,6 +38,7 @@ module Bundler
       @platforms = platforms
       @gem_version_promoter = gem_version_promoter
       @allow_bundler_dependency_conflicts = Bundler.feature_flag.allow_bundler_dependency_conflicts?
+      @lockfile_uses_separate_rubygems_sources = Bundler.feature_flag.lockfile_uses_separate_rubygems_sources?
     end
 
     def start(requirements)
@@ -145,18 +146,16 @@ module Bundler
 
     def index_for(dependency)
       source = @source_requirements[dependency.name]
-      if Bundler.feature_flag.lockfile_uses_separate_rubygems_sources?
+      if source
+        source.specs
+      elsif @lockfile_uses_separate_rubygems_sources
         Index.build do |idx|
-          if source
-            idx.add_source source.specs
-          elsif dependency.all_sources
+          if dependency.all_sources
             dependency.all_sources.each {|s| idx.add_source(s.specs) if s }
           else
             idx.add_source @source_requirements[:default].specs
           end
         end
-      elsif source
-        source.specs
       else
         @index
       end
@@ -191,7 +190,7 @@ module Bundler
     def relevant_sources_for_vertex(vertex)
       if vertex.root?
         [@source_requirements[vertex.name]]
-      elsif Bundler.feature_flag.lockfile_uses_separate_rubygems_sources?
+      elsif @lockfile_uses_separate_rubygems_sources
         vertex.recursive_predecessors.map do |v|
           @source_requirements[v.name]
         end << @source_requirements[:default]
@@ -339,7 +338,7 @@ module Bundler
               [conflict.requirement.source]
             elsif conflict.requirement.all_sources
               conflict.requirement.all_sources
-            elsif Bundler.feature_flag.lockfile_uses_separate_rubygems_sources?
+            elsif @lockfile_uses_separate_rubygems_sources
               # every conflict should have an explicit group of sources when we
               # enforce strict pinning
               raise "no source set for #{conflict}"
diff --git a/lib/bundler/source.rb b/lib/bundler/source.rb
index 956cf39d56..5a1f05098b 100644
--- a/lib/bundler/source.rb
+++ b/lib/bundler/source.rb
@@ -38,6 +38,10 @@ module Bundler
     # dependencies, looking for gems we don't have info on yet.
     def double_check_for(*); end
 
+    def dependency_names_to_double_check
+      specs.dependency_names
+    end
+
     def include?(other)
       other == self
     end
diff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb
index fa60bb0c84..6f4157364f 100644
--- a/lib/bundler/source/rubygems.rb
+++ b/lib/bundler/source/rubygems.rb
@@ -259,13 +259,36 @@ module Bundler
         return unless api_fetchers.any?
 
         unmet_dependency_names = unmet_dependency_names.call
-        return if !unmet_dependency_names.nil? && unmet_dependency_names.empty?
+        unless unmet_dependency_names.nil?
+          if api_fetchers.size <= 1
+            # can't do this when there are multiple fetchers because then we might not fetch from _all_
+            # of them
+            unmet_dependency_names -= remote_specs.spec_names # avoid re-fetching things we've already gotten
+          end
+          return if unmet_dependency_names.empty?
+        end
 
         Bundler.ui.debug "Double checking for #{unmet_dependency_names || "all specs (due to the size of the request)"} in #{self}"
 
         fetch_names(api_fetchers, unmet_dependency_names, index, override_dupes)
       end
 
+      def dependency_names_to_double_check
+        names = []
+        remote_specs.each do |spec|
+          case spec
+          when EndpointSpecification, Gem::Specification, StubSpecification, LazySpecification
+            names.concat(spec.runtime_dependencies)
+          when RemoteSpecification # from the full index
+            return nil
+          else
+            raise "unhandled spec type (#{spec.inspect})"
+          end
+        end
+        names.map!(&:name) if names
+        names
+      end
+
     protected
 
       def credless_remotes
diff --git a/lib/bundler/spec_set.rb b/lib/bundler/spec_set.rb
index 5239a96213..7cd3021997 100644
--- a/lib/bundler/spec_set.rb
+++ b/lib/bundler/spec_set.rb
@@ -110,9 +110,10 @@ module Bundler
 
     def merge(set)
       arr = sorted.dup
-      set.each do |s|
-        next if arr.any? {|s2| s2.name == s.name && s2.version == s.version && s2.platform == s.platform }
-        arr << s
+      set.each do |set_spec|
+        full_name = set_spec.full_name
+        next if arr.any? {|spec| spec.full_name == full_name }
+        arr << set_spec
       end
       SpecSet.new(arr)
     end
diff --git a/spec/install/gems/compact_index_spec.rb b/spec/install/gems/compact_index_spec.rb
index 273366c32f..f633004a3d 100644
--- a/spec/install/gems/compact_index_spec.rb
+++ b/spec/install/gems/compact_index_spec.rb
@@ -253,6 +253,22 @@ The checksum of /versions does not match the checksum provided by the server! So
     end
   end
 
+  it "does not double check for gems that are only installed locally" do
+    system_gems %w[rack-1.0.0 thin-1.0 net_a-1.0]
+    bundle! "config --local path.system true"
+    ENV["BUNDLER_SPEC_ALL_REQUESTS"] = strip_whitespace(<<-EOS).strip
+      #{source_uri}/versions
+      #{source_uri}/info/rack
+    EOS
+
+    install_gemfile! <<-G, :artifice => "compact_index", :verbose => true
+      source "#{source_uri}"
+      gem "rack"
+    G
+
+    expect(last_command.stdboth).not_to include "Double checking"
+  end
+
   it "fetches again when more dependencies are found in subsequent sources", :bundler => "< 2" do
     build_repo2 do
       build_gem "back_deps" do |s|
@@ -279,14 +295,13 @@ The checksum of /versions does not match the checksum provided by the server! So
       FileUtils.rm_rf Dir[gem_repo2("gems/foo-*.gem")]
     end
 
-    gemfile <<-G
+    install_gemfile! <<-G, :artifice => "compact_index_extra", :verbose => true
       source "#{source_uri}"
       source "#{source_uri}/extra" do
         gem "back_deps"
       end
     G
 
-    bundle! :install, :artifice => "compact_index_extra"
     expect(the_bundle).to include_gems "back_deps 1.0", "foo 1.0"
   end
 
diff --git a/spec/install/gems/dependency_api_spec.rb b/spec/install/gems/dependency_api_spec.rb
index 22c623ab40..2ffe4b62d7 100644
--- a/spec/install/gems/dependency_api_spec.rb
+++ b/spec/install/gems/dependency_api_spec.rb
@@ -320,7 +320,7 @@ RSpec.describe "gemcutter's dependency API" do
       gem 'somegem', '1.0.0'
     G
 
-    bundle :install, :artifice => "endpoint_extra_api"
+    bundle! :install, :artifice => "endpoint_extra_api"
 
     expect(the_bundle).to include_gems "somegem 1.0.0"
     expect(the_bundle).to include_gems "activesupport 1.2.3"
@@ -368,7 +368,7 @@ RSpec.describe "gemcutter's dependency API" do
 
     bundle :install, :artifice => "endpoint_extra"
 
-    expect(out).to include("Fetching gem metadata from http://localgemserver.test/..")
+    expect(out).to include("Fetching gem metadata from http://localgemserver.test/.")
     expect(out).to include("Fetching source index from http://localgemserver.test/extra")
   end
 
diff --git a/spec/support/artifice/endpoint.rb b/spec/support/artifice/endpoint.rb
index 33e2a9b411..94f2b5cbc9 100644
--- a/spec/support/artifice/endpoint.rb
+++ b/spec/support/artifice/endpoint.rb
@@ -8,11 +8,37 @@ $LOAD_PATH.unshift(*Dir[Spec::Path.base_system_gems.join("gems/{artifice,rack,ti
 require "artifice"
 require "sinatra/base"
 
+ALL_REQUESTS = [] # rubocop:disable Style/MutableConstant
+ALL_REQUESTS_MUTEX = Mutex.new
+
+at_exit do
+  if expected = ENV["BUNDLER_SPEC_ALL_REQUESTS"]
+    expected = expected.split("\n").sort
+    actual = ALL_REQUESTS.sort
+
+    unless expected == actual
+      raise "Unexpected requests!\nExpected:\n\t#{expected.join("\n\t")}\n\nActual:\n\t#{actual.join("\n\t")}"
+    end
+  end
+end
+
 class Endpoint < Sinatra::Base
+  def self.all_requests
+    @all_requests ||= []
+  end
+
   GEM_REPO = Pathname.new(ENV["BUNDLER_SPEC_GEM_REPO"] || Spec::Path.gem_repo1)
   set :raise_errors, true
   set :show_exceptions, false
 
+  def call!(*)
+    super.tap do
+      ALL_REQUESTS_MUTEX.synchronize do
+        ALL_REQUESTS << @request.url
+      end
+    end
+  end
+
   helpers do
     def dependencies_for(gem_names, gem_repo = GEM_REPO)
       return [] if gem_names.nil? || gem_names.empty?