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

Avoid making unnecessary network requests fetching specs

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

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.

### What was your diagnosis of the problem?

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_.

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

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")

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

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.

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 ecb41f83c2..4b9a781c82 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 7962ff40f8..5adc8ecbd8 100644
--- a/lib/bundler/resolver.rb
+++ b/lib/bundler/resolver.rb
@@ -39,6 +39,7 @@ module Bundler
       @gem_version_promoter = gem_version_promoter
       @allow_bundler_dependency_conflicts = Bundler.feature_flag.allow_bundler_dependency_conflicts?
       @use_gvp = Bundler.feature_flag.use_gem_version_promoter_for_major_updates? || !@gem_version_promoter.major?
+      @lockfile_uses_separate_rubygems_sources = Bundler.feature_flag.lockfile_uses_separate_rubygems_sources?
     end
 
     def start(requirements)
@@ -149,18 +150,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
@@ -195,7 +194,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]
@@ -343,7 +342,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 0d52130263..9afecff8e6 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?