From df14b3c61674554029ce3007462c1ad477b17224 Mon Sep 17 00:00:00 2001 From: Kevin Yank Date: Mon, 30 Jun 2014 12:12:54 +1000 Subject: [PATCH 1/4] Climb cause chain to determine if exception is due to Sidekiq::Shutdown. --- lib/sidekiq/middleware/server/retry_jobs.rb | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/sidekiq/middleware/server/retry_jobs.rb b/lib/sidekiq/middleware/server/retry_jobs.rb index e33f9881..7970dcfb 100644 --- a/lib/sidekiq/middleware/server/retry_jobs.rb +++ b/lib/sidekiq/middleware/server/retry_jobs.rb @@ -64,9 +64,8 @@ module Sidekiq # ignore, will be pushed back onto queue during hard_shutdown raise rescue Exception => e - # In Ruby 2.1.0 only, check if exception is a result of shutdown. - # If so, will be pushed back onto queue during hard_shutdown. - if defined?(e.cause) && e.cause.class == Sidekiq::Shutdown + if exception_caused_by_shutdown?(e) + # ignore, will be pushed back onto queue during hard_shutdown. raise Sidekiq::Shutdown end @@ -169,6 +168,15 @@ module Sidekiq end end + def exception_caused_by_shutdown?(e) + # In Ruby 2.1.0 only, check if exception is a result of shutdown. + defined?(e.cause) && + ( + e.cause.class == Sidekiq::Shutdown || + exception_caused_by_shutdown?(e.cause) + ) + end + end end end From bc63d0bea3b98cb7b812afa2286dce4f758bd26e Mon Sep 17 00:00:00 2001 From: Kevin Yank Date: Mon, 30 Jun 2014 12:40:18 +1000 Subject: [PATCH 2/4] Improve code style for exception_caused_by_shutdown? --- lib/sidekiq/middleware/server/retry_jobs.rb | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/sidekiq/middleware/server/retry_jobs.rb b/lib/sidekiq/middleware/server/retry_jobs.rb index 7970dcfb..64bb5358 100644 --- a/lib/sidekiq/middleware/server/retry_jobs.rb +++ b/lib/sidekiq/middleware/server/retry_jobs.rb @@ -64,10 +64,8 @@ module Sidekiq # ignore, will be pushed back onto queue during hard_shutdown raise rescue Exception => e - if exception_caused_by_shutdown?(e) - # ignore, will be pushed back onto queue during hard_shutdown. - raise Sidekiq::Shutdown - end + # ignore, will be pushed back onto queue during hard_shutdown + raise Sidekiq::Shutdown if exception_caused_by_shutdown?(e) raise e unless msg['retry'] max_retry_attempts = retry_attempts_from(msg['retry'], @max_retries) @@ -170,11 +168,10 @@ module Sidekiq def exception_caused_by_shutdown?(e) # In Ruby 2.1.0 only, check if exception is a result of shutdown. - defined?(e.cause) && - ( - e.cause.class == Sidekiq::Shutdown || + return false unless defined?(e.cause) + + e.cause.instance_of?(Sidekiq::Shutdown) || exception_caused_by_shutdown?(e.cause) - ) end end From 025327c6f44dd515b4522a3c882d628225ae1ca2 Mon Sep 17 00:00:00 2001 From: Kevin Yank Date: Mon, 30 Jun 2014 16:58:19 +1000 Subject: [PATCH 3/4] Test handling exceptions during Sidekiq::Shutdown. --- test/test_retry.rb | 54 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/test/test_retry.rb b/test/test_retry.rb index 726220f4..e4d8e096 100644 --- a/test/test_retry.rb +++ b/test/test_retry.rb @@ -117,6 +117,60 @@ class TestRetry < Sidekiq::Test @redis.verify end + it 'shuts down without retrying work-in-progress, which will resume' do + @redis.expect :zadd, 1, ['retry', String, String] + msg = { 'class' => 'Bob', 'args' => [1,2,'foo'], 'retry' => true } + handler = Sidekiq::Middleware::Server::RetryJobs.new + assert_raises Sidekiq::Shutdown do + handler.call(worker, msg, 'default') do + raise Sidekiq::Shutdown + end + end + assert_raises(MockExpectationError, "zadd should not be called") do + @redis.verify + end + end + + it 'shuts down cleanly when shutdown causes exception' do + @redis.expect :zadd, 1, ['retry', String, String] + msg = { 'class' => 'Bob', 'args' => [1,2,'foo'], 'retry' => true } + handler = Sidekiq::Middleware::Server::RetryJobs.new + assert_raises Sidekiq::Shutdown do + handler.call(worker, msg, 'default') do + begin + raise Sidekiq::Shutdown + rescue Interrupt + raise "kerblammo!" + end + end + end + assert_raises(MockExpectationError, "zadd should not be called") do + @redis.verify + end + end + + it 'shuts down cleanly when shutdown causes chained exceptions' do + @redis.expect :zadd, 1, ['retry', String, String] + msg = { 'class' => 'Bob', 'args' => [1,2,'foo'], 'retry' => true } + handler = Sidekiq::Middleware::Server::RetryJobs.new + assert_raises Sidekiq::Shutdown do + handler.call(worker, msg, 'default') do + begin + raise Sidekiq::Shutdown + rescue Interrupt + begin + raise "kerblammo!" + rescue + raise "kablooie!" + end + end + end + end + assert_raises(MockExpectationError, "zadd should not be called") do + @redis.verify + end + end + it 'allows a retry queue' do @redis.expect :zadd, 1, ['retry', String, String] msg = { 'class' => 'Bob', 'args' => [1,2,'foo'], 'retry' => true, 'retry_queue' => 'retry' } From 5be876bb1552db62db8a878c364bb4b8647eb5af Mon Sep 17 00:00:00 2001 From: Kevin Yank Date: Mon, 30 Jun 2014 17:09:44 +1000 Subject: [PATCH 4/4] Mark tests that cannot pass under Ruby < 2.1.0 as pending on those versions. --- test/test_retry.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/test_retry.rb b/test/test_retry.rb index e4d8e096..f511f220 100644 --- a/test/test_retry.rb +++ b/test/test_retry.rb @@ -132,6 +132,8 @@ class TestRetry < Sidekiq::Test end it 'shuts down cleanly when shutdown causes exception' do + skip('Not supported in Ruby < 2.1.0') if RUBY_VERSION < '2.1.0' + @redis.expect :zadd, 1, ['retry', String, String] msg = { 'class' => 'Bob', 'args' => [1,2,'foo'], 'retry' => true } handler = Sidekiq::Middleware::Server::RetryJobs.new @@ -150,6 +152,8 @@ class TestRetry < Sidekiq::Test end it 'shuts down cleanly when shutdown causes chained exceptions' do + skip('Not supported in Ruby < 2.1.0') if RUBY_VERSION < '2.1.0' + @redis.expect :zadd, 1, ['retry', String, String] msg = { 'class' => 'Bob', 'args' => [1,2,'foo'], 'retry' => true } handler = Sidekiq::Middleware::Server::RetryJobs.new