From eb2115237d1a3529881dd03d690c20d7b4d510c5 Mon Sep 17 00:00:00 2001 From: Eugen Kuksa Date: Sat, 4 Apr 2015 09:27:13 +0200 Subject: [PATCH 1/3] Handle circular error causes Shall fix #2284. While checking whether an application error or a sidekiq shutdown caused the exception, we keep track of the causes that were already checked. If a cause was already checked, we can stop because the whole transitive closure of its causes was checked as well. --- lib/sidekiq/middleware/server/retry_jobs.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/sidekiq/middleware/server/retry_jobs.rb b/lib/sidekiq/middleware/server/retry_jobs.rb index 3e5ea0f0..2a64150f 100644 --- a/lib/sidekiq/middleware/server/retry_jobs.rb +++ b/lib/sidekiq/middleware/server/retry_jobs.rb @@ -188,12 +188,16 @@ module Sidekiq end end - def exception_caused_by_shutdown?(e) + def exception_caused_by_shutdown?(e, checked_causes = []) # In Ruby 2.1.0 only, check if exception is a result of shutdown. return false unless defined?(e.cause) + # Handle circular causes + checked_causes << e.object_id + return false if checked_causes.include?(e.cause.object_id) + e.cause.instance_of?(Sidekiq::Shutdown) || - exception_caused_by_shutdown?(e.cause) + exception_caused_by_shutdown?(e.cause, checked_causes) end end From 60b7c952d58ef3c3760af8b2db9cc713d8d9d3e5 Mon Sep 17 00:00:00 2001 From: Eugen Kuksa Date: Wed, 8 Apr 2015 13:08:29 +0200 Subject: [PATCH 2/3] Add test: infinite recursion/circular error causes --- test/test_retry.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/test_retry.rb b/test/test_retry.rb index 8320855e..7b8a3c42 100644 --- a/test/test_retry.rb +++ b/test/test_retry.rb @@ -328,6 +328,30 @@ class TestRetry < Sidekiq::Test File.read(@tmp_log_path), 'Log entry missing for sidekiq_retry_in') end end + + describe 'handles errors with circular causes' do + before do + @error = nil + begin + begin + raise ::StandardError, 'Error 1' + rescue ::StandardError => e1 + begin + raise ::StandardError, 'Error 2' + rescue ::StandardError => e2 + raise e1 + end + end + rescue ::StandardError => e + @error = e + end + end + + it "does not recurse infinitely checking if it's a shudtown" do + assert(!Sidekiq::Middleware::Server::RetryJobs.new.send( + :exception_caused_by_shutdown?, @error)) + end + end end end From 4fa9fa89607ae57cd19b01291c108cdd25aaee2b Mon Sep 17 00:00:00 2001 From: Eugen Kuksa Date: Wed, 8 Apr 2015 19:34:49 +0200 Subject: [PATCH 3/3] Add test: normal error without cause, fix typo. --- test/test_retry.rb | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/test/test_retry.rb b/test/test_retry.rb index 7b8a3c42..40894fad 100644 --- a/test/test_retry.rb +++ b/test/test_retry.rb @@ -329,6 +329,22 @@ class TestRetry < Sidekiq::Test end end + describe 'handles errors withouth cause' do + before do + @error = nil + begin + raise ::StandardError, 'Error' + rescue ::StandardError => e + @error = e + end + end + + it "does not recurse infinitely checking if it's a shutdown" do + assert(!Sidekiq::Middleware::Server::RetryJobs.new.send( + :exception_caused_by_shutdown?, @error)) + end + end + describe 'handles errors with circular causes' do before do @error = nil @@ -347,7 +363,7 @@ class TestRetry < Sidekiq::Test end end - it "does not recurse infinitely checking if it's a shudtown" do + it "does not recurse infinitely checking if it's a shutdown" do assert(!Sidekiq::Middleware::Server::RetryJobs.new.send( :exception_caused_by_shutdown?, @error)) end