Is this a good way to discard a job when it times out?

[isaac]

Can anyone see any issues with the following code I added to one of my jobs to handle timeouts?

JobTimeoutError = Class.new StandardError

around_perform do |_job, block|
  Timeout.timeout 225, JobTimeoutError do
    block.call
  rescue JobTimeoutError
    GoodJob::Job.find(_job.job_id).discard_job "Timed out"
  end
end

[bensheldon]

I wouldn't recommend calling GoodJob::Job#discard_job in the middle of job execution because I'm not sure if the job subsequently finishing would override that and it would retry (it's a private method so I would generally say "undefined behavior subject to change").

I think you could do this entirely within ActiveJob using discard_on. My recommendation would look something like this:

JobTimeoutError = Class.new StandardError

discard_on JobTimeoutError

around_perform do |_job, block|
  Timeout.timeout 225, JobTimeoutError do
    block.call
  end
end

Of course, using Ruby Timeout is unsafe because it could raise in the middle of something and thus ruin a shared resource, but I have stuff like this in my production apps and I only end up with something like an invalid Active Record connection once a year; though your mileage may vary.

[isaac]

@bensheldon thanks for the quick response!

I did try using discard_on initially, but the following around_perform block in ApplicationJob was run first. To be clear the Timeout.timeout block was in a subclass of ApplicationJob. I didn't want to report an exception in this scenario and I couldn't think of a workaround. Any ideas about how to deal with this?

around_perform do |_job, block|
  block.call
rescue Exception => exception
  Rails.error.report exception
  raise
end

[isaac]

@bensheldon never mind - I found a solution - will post it shortly.

[isaac]

@bensheldon to get discard_on to work I needed to change the code in ApplicationJob to the following. Let me know if you can think of a better way to achieve the same thing. I tried this earlier as well but exceptions were still getting reported. Then I realised that it was my staging server that was reporting the exceptions and everything was working as expected in development 🤦

around_perform do |_job, block|
  block.call
rescue JobTimeoutError
  raise
rescue Exception => exception
  Rails.error.report exception
  raise
end

Thanks for pointing me in the right direction!

FWIW this LOC GoodJob::Job.find(_job.job_id).discard_job "Timed out" seemed to work well, but I take the point that it is not a good idea to rely on private methods.

If you wanted to use a safer solution than ruby Timeout what would you replace it with?