Retry fetch specs with `--retry` #2601

schneems · 2013-08-16T18:34:26Z

This PR adds a general purpose Retry class that can be re-used where-ever retry code is needed!

It also allows you to call bundle install --retry 3 which will attempt to connect to ruby gems 3 times before failing, and emit a warning message each time.

Bundler::Retry.new("something", 3).attempts do
  # code to retry here
end

mogox · 2013-08-16T18:45:04Z

lib/bundler/retry.rb

+    end
+
+    def keep_trying?
+      return true  if @failed.nil?


Wouldn't it make sense to initialize @falied as false and remove this condition?

other than that, your PR looks great! Loved the spec.

If @failed is false then run never gets called at all. I'm using it here as a special case to run the first time. I guess I could add a return true if current_attempt == 0 instead, which might be a bit more clear.

Updated to use current_attempt.zero? which makes more sense syntactically.

xaviershay · 2013-08-20T02:12:28Z

I'm curious what the motivation for this is. Do you just have a flakey internet connection?

xaviershay · 2013-08-20T02:12:55Z

lib/bundler/retry.rb

@@ -0,0 +1,47 @@
+module Bundler
+  class Retry


Please include motivation for this class as a class level comment.

xaviershay · 2013-08-20T02:15:27Z

Oh I just read the linked PR, I see where this came from.

TimMoore · 2013-08-20T06:48:53Z

lib/bundler/installer.rb

+          @definition.resolve_with_cache!
+        else
+          Bundler::Retry.new("source fetch", retry_times).attempts do
+            @definition.resolve_remotely!


I was a little surprised to see the retry handled at this level (wrapped around the entire resolution process) rather than at a lower level (like around individual network calls or calls out to git). I'm just wondering why you decided to go that way.

My concern is that this requires resolve_remotely! to be fully idempotent (or else bugs) and to have enough internal caching that it would avoid redoing work that completed successfully on a previous try. It's not clear to me whether that's the case right now (it looks like it could be, but there's a lot going on in there) and it would be really easy to accidentally break in the future.

This was more a proof of concept showing how I could utilize the code, we could push it lower level, or i could rip out this implementation just merge in the Retry class. The key is that there are several areas where we might want to retry code, and writing retry code by hand each time makes those implementations much less clear.

schneems · 2013-09-18T15:02:01Z

Been a month, i realize priorities are towards speed for this release, but are there any additional thoughts or comments here? Is this PR being considered or should it be closed?

indirect · 2013-09-18T15:13:26Z

As a general policy, I think we should retry all failed network requests at least once, and maybe twice, by default. adding --retry to increase the number of retries allowed also seems good.

@schneems, are you interested in expanding this to include retrying gem and git fetching, or is gemspecs as far as your interest goes?

On Wed, Sep 18, 2013 at 8:02 AM, Richard Schneeman
notifications@github.com wrote:

Been a month, i realize priorities are towards speed for this release, but are there any additional thoughts or comments here? Is this PR being considered or should it be closed?

Reply to this email directly or view it on GitHub:
#2601 (comment)

schneems · 2013-09-18T15:26:50Z

Thanks for the reply. I would be happy to take a stab at retrying in those places.

indirect · 2013-09-18T16:44:38Z

Awesome. Thanks! :D

schneems · 2013-09-23T20:37:30Z

As of now the feature defaults to 3 attempts (the first plus two retries). To skip retries all together you can run with --retry 0. Any number you pass to --retry will be run one additional time. So if you run --retry 9 it will attempt to run it 10 times total (1 initial time, and 9 retries).

Added git retry functionality. As implemented it will retry all git commands. This is a pretty invasive change, but the cleanest way to implement the feature. For the gem fetching retry, is this a decent place for the retry functionality https://github.com/bundler/bundler/blob/master/lib/bundler/installer.rb#L107 ?

indirect · 2013-09-23T20:38:37Z

can we maybe just whitelist git commands that hit the network? it seems silly to run git reset --hard ten times if it fails the first time...

flyinprogrammer · 2013-09-28T17:03:06Z

this addresses #2421

indirect · 2013-09-28T18:47:02Z

If someone wants to rebase this, I am 👍 on merging.

This PR adds a general purpose Retry class that can be re-used where-ever retry code is needed! It also allows you to call `bundle install --retry 3` which will attempt to connect to ruby gems 3 times before failing, and emit a warning message each time

schneems · 2013-09-28T19:24:27Z

rebased, but we're still retrying on all git commands, haven't had time to work on some kind of whitelisting.

I also cleaned up some committed code while rebasing, using custom error classes makes the code much more readable IMHO.

indirect · 2013-09-28T19:25:28Z

I'm down, then. Thanks!

Retry fetch specs with `--retry`

flyinprogrammer · 2013-09-28T19:30:54Z

do we want to refactor this to use this new retry class?

https://github.com/bundler/bundler/blob/master/lib/bundler/fetcher.rb#L157-L192

indirect · 2013-09-28T19:32:04Z

@flyinprogramer you're welcome to give it a shot, but the Fetcher should only retry on very specific exceptions — the retry class is just a generic retry thing.

schneems · 2013-09-28T19:45:34Z

I would add an arg to the attempt method that takes an optional error
class and saves it to an @classes_to_rescue_from_exception, have it default
to Standard error if no values are given, then rescue from that instance
variable here:
https://github.com/bundler/bundler/blob/master/lib/bundler/retry.rb#L34-L40.

That way you could write something like

Bundler::Retry.new.attempts(MyCustomError, GitError) do
  # code to retry now
end

indirect · 2013-09-28T20:35:43Z

Oh lols, the tests use stabby proc >:|

schneems · 2013-09-28T21:43:22Z

Whoops was on auto pilot on that one. I'm at a wedding for the rest of the weekend :/

—
Sent from Mailbox for iPhone

On Sat, Sep 28, 2013 at 3:35 PM, André Arko notifications@github.com
wrote:

Oh lols, the tests use stabby proc >:

Reply to this email directly or view it on GitHub:
#2601 (comment)

indirect · 2013-09-28T21:45:47Z

I fixed it. Have a good weekend! 🤘

On Sat, Sep 28, 2013 at 2:43 PM, Richard Schneeman
notifications@github.com wrote:

Whoops was on auto pilot on that one. I'm at a wedding for the rest of the weekend :/
—
Sent from Mailbox for iPhone
On Sat, Sep 28, 2013 at 3:35 PM, André Arko notifications@github.com
wrote:

Oh lols, the tests use stabby proc >:

Reply to this email directly or view it on GitHub:

#2601 (comment)

Reply to this email directly or view it on GitHub:
#2601 (comment)

schneems · 2013-09-30T02:33:51Z

❤️ 💓 😍 ❤️

xbeta · 2014-04-10T23:01:51Z

+1 for this neat feature! when can we expect a PR merge?

Who828 · 2014-04-11T03:37:40Z

@xbeta the feature had already been shipped with Bundler 1.5,
http://bundler.io/v1.5/bundle_install.html#retry

Just update your Bundler to the latest version to use it :)

xbeta · 2014-04-11T06:18:00Z

@Who828 Cool! I guess I miss that part on the doc!

schneems mentioned this pull request Aug 16, 2013

[close #129] Retry bundle install up to 3 times heroku/heroku-buildpack-ruby#136

Closed

mogox reviewed Aug 16, 2013
View reviewed changes

schneems mentioned this pull request Aug 16, 2013

Added support for retrying git commands should they fail. #2594

Closed

xaviershay reviewed Aug 20, 2013
View reviewed changes

TimMoore reviewed Aug 20, 2013
View reviewed changes

flyinprogrammer mentioned this pull request Sep 28, 2013

api calls now get retried #2561

Closed

schneems added 2 commits September 28, 2013 14:16

Retry fetch specs with --retry

dc7dd9a

This PR adds a general purpose Retry class that can be re-used where-ever retry code is needed! It also allows you to call `bundle install --retry 3` which will attempt to connect to ruby gems 3 times before failing, and emit a warning message each time

Retry git commands

a5d870f

indirect added a commit that referenced this pull request Sep 28, 2013

Merge pull request #2601 from schneems/schneems/retry

e82fde4

Retry fetch specs with `--retry`

indirect merged commit e82fde4 into rubygems:master Sep 28, 2013

tomafro mentioned this pull request Nov 18, 2013

Add bundle install retry feature tomafro/recap#96

Open

schneems mentioned this pull request Nov 11, 2015

Add retry to bundle install heroku/heroku-buildpack-ruby#435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry fetch specs with `--retry` #2601

Retry fetch specs with `--retry` #2601

schneems commented Aug 16, 2013

mogox Aug 16, 2013

schneems Aug 16, 2013

schneems Aug 16, 2013

xaviershay commented Aug 20, 2013

xaviershay Aug 20, 2013

xaviershay commented Aug 20, 2013

TimMoore Aug 20, 2013

schneems Aug 20, 2013

schneems commented Sep 18, 2013

indirect commented Sep 18, 2013

Been a month, i realize priorities are towards speed for this release, but are there any additional thoughts or comments here? Is this PR being considered or should it be closed?

schneems commented Sep 18, 2013

indirect commented Sep 18, 2013

schneems commented Sep 23, 2013

indirect commented Sep 23, 2013

flyinprogrammer commented Sep 28, 2013

indirect commented Sep 28, 2013

schneems commented Sep 28, 2013

indirect commented Sep 28, 2013

flyinprogrammer commented Sep 28, 2013

indirect commented Sep 28, 2013

schneems commented Sep 28, 2013

indirect commented Sep 28, 2013

schneems commented Sep 28, 2013

indirect commented Sep 28, 2013

#2601 (comment)

schneems commented Sep 30, 2013

xbeta commented Apr 10, 2014

Who828 commented Apr 11, 2014

xbeta commented Apr 11, 2014

Retry fetch specs with --retry #2601

Retry fetch specs with --retry #2601

Conversation

schneems commented Aug 16, 2013

mogox Aug 16, 2013

Choose a reason for hiding this comment

schneems Aug 16, 2013

Choose a reason for hiding this comment

schneems Aug 16, 2013

Choose a reason for hiding this comment

xaviershay commented Aug 20, 2013

xaviershay Aug 20, 2013

Choose a reason for hiding this comment

xaviershay commented Aug 20, 2013

TimMoore Aug 20, 2013

Choose a reason for hiding this comment

schneems Aug 20, 2013

Choose a reason for hiding this comment

schneems commented Sep 18, 2013

indirect commented Sep 18, 2013

Been a month, i realize priorities are towards speed for this release, but are there any additional thoughts or comments here? Is this PR being considered or should it be closed?

schneems commented Sep 18, 2013

indirect commented Sep 18, 2013

schneems commented Sep 23, 2013

indirect commented Sep 23, 2013

flyinprogrammer commented Sep 28, 2013

indirect commented Sep 28, 2013

schneems commented Sep 28, 2013

indirect commented Sep 28, 2013

flyinprogrammer commented Sep 28, 2013

indirect commented Sep 28, 2013

schneems commented Sep 28, 2013

indirect commented Sep 28, 2013

schneems commented Sep 28, 2013

indirect commented Sep 28, 2013

#2601 (comment)

schneems commented Sep 30, 2013

xbeta commented Apr 10, 2014

Who828 commented Apr 11, 2014

xbeta commented Apr 11, 2014

Retry fetch specs with `--retry` #2601

Retry fetch specs with `--retry` #2601