From 95d9c3b3d6828b8ce37591e68d4239ce8c18460b Mon Sep 17 00:00:00 2001 From: Alberto Almagro Date: Fri, 23 Nov 2018 20:31:14 +0100 Subject: Keep executions for each specific exception (#34352) * Keep executions for each specific declaration Fixes #34337 ActiveJob used the global executions counter to control the number of times a job should be retried. The problem with this approach was that in case a job raised different exceptions during its executions they weren't retried the number of times defined by their `attemps` number. **Example:** Having the following job: ```ruby class BuggyJob < ActiveJob::Base retry_on CustomException, attemps: 3 retry_on OtherException, attempts: 3 end ``` If the job raised `CustomException` in the first two executions and then it raised `OtherException`, the job wasn't retried anymore because the global executions counter was already indicating 3 attempts. With this patch each `retry_on` declaration has its specific counter so that the first two executions that raise `CustomException` don't affect the retries count that future exceptions may have. * Revert "clarifies documentation around the attempts arugment to retry_on" This reverts commit 86aa8f8c5631f77ed9a208e5107003c01512133e. --- activejob/CHANGELOG.md | 7 +++++++ activejob/lib/active_job/core.rb | 9 +++++++++ activejob/lib/active_job/exceptions.rb | 8 ++++---- activejob/test/cases/exceptions_test.rb | 22 ++++++++++++++++++++++ activejob/test/jobs/retry_job.rb | 9 +++++++++ 5 files changed, 51 insertions(+), 4 deletions(-) (limited to 'activejob') diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index e5cbff4a88..8e18faeb02 100644 --- a/activejob/CHANGELOG.md +++ b/activejob/CHANGELOG.md @@ -1,3 +1,10 @@ +* Keep executions for each specific declaration + + Each `retry_on` declaration has now its own specific executions counter. Before it was + shared between all executions of a job. + + *Alberto Almagro* + * Allow all assertion helpers that have a `only` and `except` keyword to accept Procs. diff --git a/activejob/lib/active_job/core.rb b/activejob/lib/active_job/core.rb index 698153636b..4ab62f89b0 100644 --- a/activejob/lib/active_job/core.rb +++ b/activejob/lib/active_job/core.rb @@ -28,6 +28,12 @@ module ActiveJob # Number of times this job has been executed (which increments on every retry, like after an exception). attr_accessor :executions + # Hash that contains the number of times this job handled errors for each specific retry_on declaration. + # Keys are the string representation of the exceptions listed in the retry_on declaration, + # while its associated value holds the number of executions where the corresponding retry_on + # declaration handled one of its listed exceptions. + attr_accessor :exception_executions + # I18n.locale to be used during the job. attr_accessor :locale @@ -75,6 +81,7 @@ module ActiveJob @queue_name = self.class.queue_name @priority = self.class.priority @executions = 0 + @exception_executions = Hash.new(0) end # Returns a hash with the job data that can safely be passed to the @@ -88,6 +95,7 @@ module ActiveJob "priority" => priority, "arguments" => serialize_arguments_if_needed(arguments), "executions" => executions, + "exception_executions" => exception_executions, "locale" => I18n.locale.to_s, "timezone" => Time.zone.try(:name) } @@ -126,6 +134,7 @@ module ActiveJob self.priority = job_data["priority"] self.serialized_arguments = job_data["arguments"] self.executions = job_data["executions"] + self.exception_executions = job_data["exception_executions"] self.locale = job_data["locale"] || I18n.locale.to_s self.timezone = job_data["timezone"] || Time.zone.try(:name) end diff --git a/activejob/lib/active_job/exceptions.rb b/activejob/lib/active_job/exceptions.rb index bc9e168971..53984a4e49 100644 --- a/activejob/lib/active_job/exceptions.rb +++ b/activejob/lib/active_job/exceptions.rb @@ -9,7 +9,6 @@ module ActiveJob module ClassMethods # Catch the exception and reschedule job for re-execution after so many seconds, for a specific number of attempts. - # The number of attempts includes the total executions of a job, not just the retried executions. # If the exception keeps getting raised beyond the specified number of attempts, the exception is allowed to # bubble up to the underlying queuing system, which may have its own retry mechanism or place it in a # holding queue for inspection. @@ -22,8 +21,7 @@ module ActiveJob # as a computing proc that the number of executions so far as an argument, or as a symbol reference of # :exponentially_longer, which applies the wait algorithm of (executions ** 4) + 2 # (first wait 3s, then 18s, then 83s, etc) - # * :attempts - Re-enqueues the job the specified number of times (default: 5 attempts), - # attempts here refers to the total number of times the job is executed, not just retried executions + # * :attempts - Re-enqueues the job the specified number of times (default: 5 attempts) # * :queue - Re-enqueues the job on a different queue # * :priority - Re-enqueues the job with a different priority # @@ -46,7 +44,9 @@ module ActiveJob # end def retry_on(*exceptions, wait: 3.seconds, attempts: 5, queue: nil, priority: nil) rescue_from(*exceptions) do |error| - if executions < attempts + exception_executions[exceptions.to_s] += 1 + + if exception_executions[exceptions.to_s] < attempts retry_job wait: determine_delay(wait), queue: queue, priority: priority, error: error else if block_given? diff --git a/activejob/test/cases/exceptions_test.rb b/activejob/test/cases/exceptions_test.rb index 37bb65538a..b5328b8d6a 100644 --- a/activejob/test/cases/exceptions_test.rb +++ b/activejob/test/cases/exceptions_test.rb @@ -30,6 +30,28 @@ class ExceptionsTest < ActiveJob::TestCase end end + test "keeps the same attempts counter when several exceptions are listed in the same declaration" do + exceptions_to_raise = %w(FirstRetryableErrorOfTwo FirstRetryableErrorOfTwo FirstRetryableErrorOfTwo + SecondRetryableErrorOfTwo SecondRetryableErrorOfTwo) + + assert_raises SecondRetryableErrorOfTwo do + perform_enqueued_jobs do + ExceptionRetryJob.perform_later(exceptions_to_raise) + end + end + end + + test "keeps a separate attempts counter for each individual declaration" do + exceptions_to_raise = %w(FirstRetryableErrorOfTwo FirstRetryableErrorOfTwo FirstRetryableErrorOfTwo + DefaultsError DefaultsError) + + assert_nothing_raised do + perform_enqueued_jobs do + ExceptionRetryJob.perform_later(exceptions_to_raise) + end + end + end + test "failed retry job when exception kept occurring against defaults" do perform_enqueued_jobs do begin diff --git a/activejob/test/jobs/retry_job.rb b/activejob/test/jobs/retry_job.rb index 68dc17e16c..2d19d4c41e 100644 --- a/activejob/test/jobs/retry_job.rb +++ b/activejob/test/jobs/retry_job.rb @@ -39,3 +39,12 @@ class RetryJob < ActiveJob::Base end end end + +class ExceptionRetryJob < ActiveJob::Base + retry_on FirstRetryableErrorOfTwo, SecondRetryableErrorOfTwo, attempts: 4 + retry_on DefaultsError + + def perform(exceptions) + raise exceptions.shift.constantize.new unless exceptions.empty? + end +end -- cgit v1.2.3