lantins / resque-retry

A resque plugin; provides retry, delay and exponential backoff support for resque jobs.
MIT License
421 stars 139 forks source link

retry_criteria_check is not triggered if @retry_exceptions and @fatal_exceptions are unset #145

Open oehlschl opened 7 years ago

oehlschl commented 7 years ago

retry_criteria_check methods are not triggered if both @retry_exceptions and @fatal_exceptions are nil per lines:

https://github.com/lantins/resque-retry/blob/v1.5.0/lib/resque/plugins/retry.rb#L275 https://github.com/lantins/resque-retry/blob/v1.5.0/lib/resque/plugins/retry.rb#L199

This case seems like a common one (defining a criteria check method without whitelisting or blacklisting any exception classes), and the behavior is not at all intuitive: the only constraint I define is not called because I didn't define others. For now, setting @retry_exceptions = [] is a sufficient workaround to fail the nil check without introducing unintended consequences, but retry_exceptions should not be required like this.

Never calls retry_criteria_check, always re-retries (up to limit):

  extend Resque::Plugins::Retry

  @retry_limit = 3
  @retry_delay = 60
  retry_criteria_check do |exception, *args|
    exception.is_a?(Mandrill::Error) && exception.message.include?('504 Gateway Time-out')
  end

Works as expected, calling retry_criteria_check logic:

  extend Resque::Plugins::Retry

  @retry_limit = 3
  @retry_delay = 60
  @retry_exceptions = []
  retry_criteria_check do |exception, *args|
    exception.is_a?(Mandrill::Error) && exception.message.include?('504 Gateway Time-out')
  end
davidcpell commented 6 years ago

Just ran into this myself. From the README I was expecting to be able to not specify @retry_exceptions and then have any exception run through my retry_criteria_check.

davidcpell commented 6 years ago

Update: I added @retry_exceptions = [x, y] and the retry_criteria_check still seems not to be running at all. I put some debug logs in there and am not getting them.

jzaleski commented 6 years ago

If someone would like to submit a PR to buff up the retry_criteria_valid? method (e.g. change the ordering to support this case) I'd be happy to review / merge if appropriate. I would not be opposed to short-circuiting if that solves the problem.