search

isaacseymour / activejob-retry

Automatic retries for ActiveJob
MIT License
138 stars 14 forks source link

Inheritance issue #36

Closed tdeo closed 8 years ago

tdeo commented 8 years ago

Hi,

I just came across a pretty odd issue If I have class inheritance in between my jobs. Let's say I have 2 job classes that are pretty similar, but that I want to run on different queues for priority reasons, I'll make a generic class from which both inherit, and override the queue_as method.

However, if I implement a constant_retry in the mother class, to have it in only one place, I receive those exceptions instead of the retries:

A NoMethodError occurred in background at 2015-12-18 05:28:20 UTC :

undefined method `should_retry?' for nil:NilClass

/srv/http/admin/releases/ab3ca489c158d09f6a4109956c1c3dbd5590e449/vendor/bundle/ruby/2.2.0/gems/activejob-retry-0.5.1/lib/active_job/retry.rb:94:in `rescue_with_handler'

Checking manually, the backoff strategy is indeed not available in the child classes. Any idea how we could inherit retry strategy from the mother class ?

isaacseymour commented 8 years ago

I never really thought about how this would interact with inheritance, but this kind of thing normally involves some horrible workarounds. The issue here is that, if we have:

class AJob < ActiveJob::Base
  include ActiveJob::Retry
  constant_retry ...
end

class BJob < AJob; end
class CJob < AJob; end

then BJob.new.class == BJob, when the retry settings live on AJob. I know it is possible to set up a hook so that when BJob inherits from AJob it would copy the necessary class variables over, but can't remember exactly how... I'm no longer actively working on this gem, but PRs are very welcome if you have time! This PR on gocardless/statesman implements this feature.

charlesdg commented 8 years ago

+1

isaacseymour commented 8 years ago

38 should fix this - although it means that subclasses can't remove ActiveJob::Retry, unless they do something like include ActiveJob::Retry.new(strategy: :constant, limit: 1). @tdeo /@charlesdg - I'd be very grateful if you could check that this fix does work for you!

tdeo commented 8 years ago

That indeed fixes the issue for me. Thanks

isaacseymour commented 8 years ago

Awesome! I'll get it on RubyGems in a minute...

isaacseymour commented 8 years ago

Released in 0.6.1