diff options
author | The Bundler Bot <bot@bundler.io> | 2017-08-11 15:27:05 +0000 |
---|---|---|
committer | Samuel Giddins <segiddins@segiddins.me> | 2017-08-19 11:52:54 -0400 |
commit | 5a06631c9ad68723769eb6c2c2660cf965e7eb55 (patch) | |
tree | 3ebda998943465a1650b4f48557be83d46e68fbb | |
parent | ad6cbddb497ba7890cf4896c8f155e6fef77bf8b (diff) | |
download | bundler-5a06631c9ad68723769eb6c2c2660cf965e7eb55.tar.gz |
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)
-rw-r--r-- | lib/bundler/rubygems_integration.rb | 6 | ||||
-rw-r--r-- | 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") } |