Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

download_gem not being retried #4846

Closed
jkeiser opened this issue Aug 5, 2016 · 5 comments
Closed

download_gem not being retried #4846

jkeiser opened this issue Aug 5, 2016 · 5 comments

Comments

@jkeiser
Copy link
Contributor

jkeiser commented Aug 5, 2016

It appears that retry applies to fetching specs and talking to git, but not to actually retrieving the gems themselves. We have received this error multiple times in the past month, even with bundle install ... --retry 5:

Gem::RemoteFetcher::FetchError: Errno::ECONNRESET: Connection reset by peer - SSL_connect

Bundler 1.12.5, Rubygems 2.6.5, Ruby 2.1.8p440.

For one example:

Fetching gem metadata from https://rubygems.org/
Fetching version metadata from https://rubygems.org/
Fetching dependency metadata from https://rubygems.org/
Fetching git://github.com/chef/omnibus.git
Fetching git://github.com/chef/omnibus-software.git
Fetching https://github.com/ksubrama/pedump.git
Installing awesome_print 1.7.0
Installing jmespath 1.3.1
Installing fuzzyurl 0.8.0
Installing mixlib-config 2.2.1
Installing mixlib-shellout 2.2.6
Installing chef-sugar 3.4.0
Installing cleanroom 1.0.0
Installing ffi 1.9.14 with native extensions
Installing libyajl2 1.2.0 with native extensions
Installing iostruct 0.0.4
Installing ipaddress 0.8.3
Installing mixlib-cli 1.7.0
Installing mixlib-log 1.6.0
Installing mixlib-versioning 1.1.0
Installing multipart-post 1.2.0
Installing plist 3.2.0
Installing systemu 2.6.5
Installing wmi-lite 1.0.0

Gem::RemoteFetcher::FetchError: Errno::ECONNRESET: Connection reset by peer - SSL_connect (https://rubygems.org/gems/ruby-progressbar-1.8.1.gem)
Installing thor 0.19.1
Installing progressbar 0.21.0
Installing zhexdump 0.0.2
Using bundler 1.12.5
Installing aws-sdk-core 2.5.0
Installing chef-config 12.12.15
Installing ffi-yajl 2.3.0 with native extensions
An error occurred while installing ruby-progressbar (1.8.1), and Bundler cannot
continue.
Make sure that `gem install ruby-progressbar -v '1.8.1'` succeeds before
bundling.

I can't find the code that's supposed to retry ECONNRESET (or indeed any rubygems request): rubygems' Gem::RemoteFetcher doesn't seem to take a parameter for retries, nor is bundler passing one.: https://github.com/bundler/bundler/blob/28ded71b5c399fde3ecbbe785f6077312f2c54c7/lib/bundler/rubygems_integration.rb#L277-L281

Since it happens intermittently, it's a little hard to repro, but we do enough builds to find it often. #4729 seems to have hit it as well.

@segiddins
Copy link
Member

segiddins commented Aug 5, 2016

can you try running with DEBUG=1 so we can get a backtrace on that? Thanks!

The fix is likely going to be wrapping https://github.com/bundler/bundler/blob/c85b597e81fccb5be4db6c1718de4768ad653345/lib/bundler/installer/parallel_installer.rb#L92 in a Bundler::Retry block

@jkeiser
Copy link
Contributor Author

jkeiser commented Aug 8, 2016

@segiddins sure. I'll set my builds to doing that. Not sure how long it'll take to get a repro!

@indirect
Copy link
Member

indirect commented Aug 8, 2016

@segiddins if install_from_gem doesn't have any internal retry logic, we should definitely wrap it in a retry block 😅

@scotthain
Copy link

@segiddins @indirect Here's some debug output from one of our freebsd machines. We have had a few other repros today so let @jkeiser or myself know if you need more! https://gist.github.com/scotthain/58da989cfd632f252e3d1f7c20a2919e

@indirect indirect mentioned this issue Aug 11, 2016
23 tasks
@jkeiser
Copy link
Contributor Author

jkeiser commented Aug 11, 2016

The relevant bit:

Gem::RemoteFetcher::FetchError: Errno::ECONNRESET: Connection reset by peer - SSL_connect (https://rubygems.org/gems/multipart-post-1.2.0.gem)
/opt/angry-omnibus-toolchain/embedded/lib/ruby/site_ruby/2.1.0/rubygems/remote_fetcher.rb:308:in `rescue in fetch_path'
/opt/angry-omnibus-toolchain/embedded/lib/ruby/site_ruby/2.1.0/rubygems/remote_fetcher.rb:281:in `fetch_path'
/opt/angry-omnibus-toolchain/embedded/lib/ruby/site_ruby/2.1.0/rubygems/remote_fetcher.rb:324:in `cache_update_path'
/opt/angry-omnibus-toolchain/embedded/lib/ruby/site_ruby/2.1.0/rubygems/remote_fetcher.rb:188:in `download'
/opt/angry-omnibus-toolchain/embedded/lib/ruby/gems/2.1.0/gems/bundler-1.12.5/lib/bundler/rubygems_integration.rb:615:in `download_gem'
/opt/angry-omnibus-toolchain/embedded/lib/ruby/gems/2.1.0/gems/bundler-1.12.5/lib/bundler/source/rubygems.rb:410:in `fetch_gem'
/opt/angry-omnibus-toolchain/embedded/lib/ruby/gems/2.1.0/gems/bundler-1.12.5/lib/bundler/source/rubygems.rb:116:in `install'
/opt/angry-omnibus-toolchain/embedded/lib/ruby/gems/2.1.0/gems/bundler-1.12.5/lib/bundler/installer/gem_installer.rb:57:in `install'
/opt/angry-omnibus-toolchain/embedded/lib/ruby/gems/2.1.0/gems/bundler-1.12.5/lib/bundler/installer/gem_installer.rb:15:in `install_from_spec'

jkeiser added a commit to jkeiser/bundler that referenced this issue Aug 11, 2016
jkeiser added a commit to jkeiser/bundler that referenced this issue Aug 11, 2016
homu added a commit that referenced this issue Aug 14, 2016
segiddins pushed a commit that referenced this issue Aug 19, 2016
bundlerbot added a commit that referenced this issue Aug 11, 2017
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(#4846). Also added specs that makes sure retry logic is intact, which otherwise would have failed without the current logic in place.
segiddins pushed a commit that referenced this issue Aug 19, 2017
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(#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 c14d59a)
segiddins pushed a commit that referenced this issue Aug 19, 2017
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(#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 c14d59a)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants