0
\$\begingroup\$

One of my clients sends quite a few emails a day ... I would think over 500. They are mostly reports to clients or employees. The volume has become sufficient that intermittent issues with gmail (like the occasional and annoying "please try again later" error, or "too many logins" error) are causing missed reports and headaches. We also have some issues with the MapQuest API and this might help.

So I'm writing this class to encapsulate the process of the immediate retry (as opposed to the "give it back to Resque to try again later" process.)

Dependencies:

  • Uses my gem Valuable which makes modeling easier.

Feedback welcome.

class Retryer < Valuable
  has_value :attempts, :default => 3
  has_value :logger, default: lambda { Rails.logger }

  has_value :tries, :default => 0

  has_collection :reraise, :default => [NotImplementedError]

  def fire
    raise "You must provide a block of code to try" unless block_given?

    begin
      self.tries += 1
      yield
    rescue Timeout::Error => err
      logger && logger.warn("    - Timeout!")
      sleep(0.5)
      retry if self.tries < self.attempts
    rescue => err
      if reraise.include? err.class
        logger && logger.warn("    - try #{self.tries} of #{attempts} failed and will be reraised: #{err}")
        raise err
      else
        logger && logger.warn("    - try #{self.tries} of #{attempts} failed with #{err}")
        retry if self.tries < self.attempts
      end
    end
  end
end

One thing I'd like to add is the option to run another proc/block if all attempts raise. For instance, notify Rollbar. I imagine it working somewhat like js promises. But I would only do it if I could figure out a readable interface...

\$\endgroup\$
6
  • \$\begingroup\$ On review, one issue I see is that retrying from simultaneous processes would cause confusing logs. \$\endgroup\$
    – MustModify
    Jul 10, 2017 at 17:39
  • \$\begingroup\$ Aren't the self. prefixes superfluous, fire being an instance method. also, where is retry defined? \$\endgroup\$
    – Jonah
    Jul 10, 2017 at 18:44
  • \$\begingroup\$ I've found self to be a useful reminder to me that it's a class method. But they are superfluous with respect to the compiler. retry is a Ruby keyword. \$\endgroup\$
    – MustModify
    Jul 11, 2017 at 20:00
  • \$\begingroup\$ Ah, I wasn't aware of retry. Re: self, I'm struggling to see how this would be a class method. Aren't you creating Retryer instances? Doesn't each individual instance have its own tries count, eg? \$\endgroup\$
    – Jonah
    Jul 11, 2017 at 20:04
  • \$\begingroup\$ My mistake. What I meant was that it's an instance method as opposed to a variable. Really wish I could edit that to avoid my looking stupid, but I can see it would make the comments incongruous. \$\endgroup\$
    – MustModify
    Jul 12, 2017 at 21:03

1 Answer 1

1
\$\begingroup\$

Other than the cosmetic suggestion we discussed about removing the unnecessary self prefixes, my central critique is that you're mixing concerns in this code.

Currently, you have specific logging strategies and even logging messages hardcoded in the retryer. Which will make it hard to use the class in other contexts.

The retryer should do just one thing: retry X number of times and return a result.

Exactly what you want the return values to be is up to you (you could even make a RetryResult class), but one simple option would be an array of results from each attempt: true would be the final array value if it eventually succeeds; and the array would rescued errors from each failed attempt.

The calling code could then decide how to process that information: logging it, sending emails, ignoring it, etc.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.