From 5a06631c9ad68723769eb6c2c2660cf965e7eb55 Mon Sep 17 00:00:00 2001
From: The Bundler Bot <bot@bundler.io>
Date: Fri, 11 Aug 2017 15:27:05 +0000
Subject: Auto merge of #5928 - shayonj:s/download-retry, r=segiddins

Retry downloading gems consistently across all versions of RubyGems

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

`Gem::RemoteFetcher::FetchError` failures were not being retried when downloading gems. Retrying downloading of gems for network blips is very ideal and helpful in CI environment.

### What was your diagnosis of the problem?

- The retry logic that existed (using `Bundler::Retry`) for `Bundler::RubygemsIntegration#download_gem` was not getting called, becase `Bundler.rubygems` defaulted to a sub class of `Bundler::RubygemsIntegration::MoreFuture`, which inherited from `Bundler::RubygemsIntegration::Future` and `#download_gem` in there didn't perform download with retry logic.
- Sending `Gem::RemoteFetcher::FetchError` as an argument to `Bundler::Retry` meant, `Bundler::Retry` would fail first thing,[ if exception being raised was the one being supplied](https://github.com/bundler/bundler/blob/5a61b65ad5ec1df1539ecf8bc5d124f2b254ba14/lib/bundler/retry.rb#L46..L49). The intent is to retry when downloading fails.

**Example of how sub classing issue looked like:**

```ruby
class RubygemsIntegration
  def download_gem
    "I retry when downloading"
  end
end

class Future < RubygemsIntegration
  def download_gem
    "I don't retry when downloading"
  end
end

class MoreFuture < Future
  # no download_gem
end

mf = MoreFuture.new #=> Bundler.rubygems where RubyGem version is > 2.1.0
mf.download_gem
 => "I don't retry when downloading"
```

**Example of Bundler::Retry**

```ruby
Bundler::Retry.new("download gem from", StandardError).attempts do
  puts "I will only print once, whereas I should print 4 times"
  raise StandardError.new("FooBar")
end
```
### What is your fix for the problem, implemented in this PR?

I added the retry logic for to `Bundler::RubygemsIntegration::Future#download_gem` and removed `Gem::RemoteFetcher::FetchError` as an argument for failed exceptions.

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

I chose this fix because this fix fulfills the expected behavior(https://github.com/bundler/bundler/issues/4846). Also added specs that makes sure retry logic is intact, which otherwise would have failed without the current logic in place.

(cherry picked from commit c14d59a29739ea1a4bb73f664fc6620a711250b0)
---
 lib/bundler/rubygems_integration.rb       |  6 ++++--
 spec/bundler/rubygems_integration_spec.rb | 25 +++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/lib/bundler/rubygems_integration.rb b/lib/bundler/rubygems_integration.rb
index 2e4d01ffb9..c3e16e086c 100644
--- a/lib/bundler/rubygems_integration.rb
+++ b/lib/bundler/rubygems_integration.rb
@@ -300,7 +300,7 @@ module Bundler
     def download_gem(spec, uri, path)
       uri = Bundler.settings.mirror_for(uri)
       fetcher = Gem::RemoteFetcher.new(configuration[:http_proxy])
-      Bundler::Retry.new("download gem #{uri}", Gem::RemoteFetcher::FetchError).attempts do
+      Bundler::Retry.new("download gem from #{uri}").attempts do
         fetcher.download(spec, uri, path)
       end
     end
@@ -736,7 +736,9 @@ module Bundler
         uri = Bundler.settings.mirror_for(uri)
         fetcher = gem_remote_fetcher
         fetcher.headers = { "X-Gemfile-Source" => spec.remote.original_uri.to_s } if spec.remote.original_uri
-        fetcher.download(spec, uri, path)
+        Bundler::Retry.new("download gem from #{uri}").attempts do
+          fetcher.download(spec, uri, path)
+        end
       end
 
       def gem_remote_fetcher
diff --git a/spec/bundler/rubygems_integration_spec.rb b/spec/bundler/rubygems_integration_spec.rb
index 37eb499e38..38ff9dae7e 100644
--- a/spec/bundler/rubygems_integration_spec.rb
+++ b/spec/bundler/rubygems_integration_spec.rb
@@ -54,6 +54,31 @@ RSpec.describe Bundler::RubygemsIntegration do
     end
   end
 
+  describe "#download_gem", :rubygems => ">= 2.0" do
+    let(:bundler_retry) { double(Bundler::Retry) }
+    let(:retry) { double("Bundler::Retry") }
+    let(:uri) {  URI.parse("https://foo.bar") }
+    let(:path) { Gem.path.first }
+    let(:spec) do
+      spec = Bundler::RemoteSpecification.new("Foo", Gem::Version.new("2.5.2"),
+        Gem::Platform::RUBY, nil)
+      spec.remote = Bundler::Source::Rubygems::Remote.new(uri.to_s)
+      spec
+    end
+    let(:fetcher) { double("gem_remote_fetcher") }
+
+    it "succesfully downloads gem with retries" do
+      expect(Bundler.rubygems).to receive(:gem_remote_fetcher).and_return(fetcher)
+      expect(fetcher).to receive(:headers=).with("X-Gemfile-Source" => "https://foo.bar")
+      expect(Bundler::Retry).to receive(:new).with("download gem from #{uri}/").
+        and_return(bundler_retry)
+      expect(bundler_retry).to receive(:attempts).and_yield
+      expect(fetcher).to receive(:download).with(spec, uri, path)
+
+      Bundler.rubygems.download_gem(spec, uri, path)
+    end
+  end
+
   describe "#fetch_all_remote_specs", :rubygems => ">= 2.0" do
     let(:uri) { URI("https://example.com") }
     let(:fetcher) { double("gem_remote_fetcher") }
-- 
cgit v1.2.1