Merge pull request #325 from subelsky/integrated_exception_handling

extract error handling middleware into processor-level code

lib/sidekiq/exception_handler.rb

@@ -0,0 +1,33 @@
+require 'sidekiq/util'
+
+module Sidekiq
+  module ExceptionHandler
+    extend self
+    extend Util
+
+    def handle(ex,msg)
+      logger.warn ex
+      logger.warn ex.backtrace.join("\n")
+      send_to_airbrake(msg, ex) if defined?(::Airbrake)
+      send_to_exceptional(msg, ex) if defined?(::Exceptional)
+      send_to_exception_notifier(msg, ex) if defined?(::ExceptionNotifier)
+    end
+
+    private
+
+    def send_to_airbrake(msg, ex)
+      ::Airbrake.notify(ex, :parameters => msg)
+    end
+
+    def send_to_exceptional(msg, ex)
+      if ::Exceptional::Config.should_send_to_api?
+        ::Exceptional.context(msg)
+        ::Exceptional::Remote.error(::Exceptional::ExceptionData.new(ex))
+      end
+    end
+
+    def send_to_exception_notifier(msg, ex)
+      ::ExceptionNotifier::Notifier.background_exception_notification(ex, :data => { :message => msg })
+    end
+  end
+end

lib/sidekiq/middleware/server/exception_handler.rb

@@ -1,38 +0,0 @@
-require 'sidekiq/util'
-
-module Sidekiq
-  module Middleware
-    module Server
-      class ExceptionHandler
-        include Util
-        def call(*args)
-          yield
-        rescue => ex
-          logger.warn ex
-          logger.warn ex.backtrace.join("\n")
-          send_to_airbrake(args[1], ex) if defined?(::Airbrake)
-          send_to_exceptional(args[1], ex) if defined?(::Exceptional)
-          send_to_exception_notifier(args[1], ex) if defined?(::ExceptionNotifier)
-          raise
-        end
-
-        private
-
-        def send_to_airbrake(msg, ex)
-          ::Airbrake.notify(ex, :parameters => msg)
-        end
-
-        def send_to_exceptional(msg, ex)
-          if ::Exceptional::Config.should_send_to_api?
-            ::Exceptional.context(msg)
-            ::Exceptional::Remote.error(::Exceptional::ExceptionData.new(ex))
-          end
-        end
-
-        def send_to_exception_notifier(msg, ex)
-          ::ExceptionNotifier::Notifier.background_exception_notification(ex, :data => { :message => msg })
-        end
-      end
-    end
-  end
-end

lib/sidekiq/processor.rb

@@ -2,10 +2,10 @@ require 'celluloid'
 require 'sidekiq/util'
 
 require 'sidekiq/middleware/server/active_record'
-require 'sidekiq/middleware/server/exception_handler'
 require 'sidekiq/middleware/server/retry_jobs'
 require 'sidekiq/middleware/server/logging'
 require 'sidekiq/middleware/server/timeout'
+require 'sidekiq/exception_handler'
 
 module Sidekiq
   ##
@@ -17,10 +17,10 @@ module Sidekiq
     include Celluloid
 
     exclusive :process
+    attr_writer :exception_handler
 
     def self.default_middleware
       Middleware::Chain.new do |m|
-        m.add Middleware::Server::ExceptionHandler
         m.add Middleware::Server::Logging
         m.add Middleware::Server::RetryJobs
         m.add Middleware::Server::ActiveRecord
@@ -43,6 +43,9 @@ module Sidekiq
         end
       end
       @boss.processor_done!(current_actor)
+    rescue StandardError => ex
+      exception_handler.handle(ex,msg)
+      raise
     end
 
     # See http://github.com/tarcieri/celluloid/issues/22
@@ -93,5 +96,9 @@ module Sidekiq
     def hostname
       @h ||= `hostname`.strip
     end
+
+    def exception_handler
+      @exception_handler ||= Sidekiq::ExceptionHandler
+    end
   end
 end

test/test_exception_handler.rb

@@ -0,0 +1,104 @@
+require 'helper'
+require 'sidekiq'
+require 'sidekiq/exception_handler'
+require 'stringio'
+require 'logger'
+
+ExceptionHandlerTestException = Class.new(StandardError)
+TEST_EXCEPTION = ExceptionHandlerTestException.new("Something didn't work!")
+
+def invoke_exception(args)
+  raise TEST_EXCEPTION
+rescue ExceptionHandlerTestException => e
+  Sidekiq::ExceptionHandler.handle(e,args)
+end
+
+class TestExceptionHandler < MiniTest::Unit::TestCase
+  describe "with mock logger" do
+    before do
+      @old_logger = Sidekiq.logger
+      @str_logger = StringIO.new
+      Sidekiq.logger = Logger.new(@str_logger)
+    end
+
+    after do
+      Sidekiq.logger = @old_logger
+    end
+
+    it "logs the exception to Sidekiq.logger" do
+      invoke_exception(:a => 1)
+      @str_logger.rewind
+      log = @str_logger.readlines
+      assert_match /Something didn't work!/, log[0], "didn't include the exception message"
+      assert_match /test\/test_exception_handler.rb/, log[1], "didn't include the backtrace"
+    end
+  end
+
+  describe "with fake Airbrake" do
+    before do
+      ::Airbrake = MiniTest::Mock.new
+    end
+
+    after do
+      Object.send(:remove_const, "Airbrake") # HACK should probably inject Airbrake etc into this class in the future
+    end
+
+    it "notifies Airbrake" do
+      ::Airbrake.expect(:notify,nil,[TEST_EXCEPTION,:parameters => { :a => 1 }])
+      invoke_exception(:a => 1)
+      ::Airbrake.verify
+    end
+  end
+
+  describe "with fake ExceptionNotifier" do
+    before do
+      ::ExceptionNotifier = Module.new
+      ::ExceptionNotifier::Notifier = MiniTest::Mock.new
+    end
+
+    after do
+      Object.send(:remove_const, "ExceptionNotifier")
+    end
+
+    it "notifies ExceptionNotifier" do
+      ::ExceptionNotifier::Notifier.expect(:background_exception_notification,nil,[TEST_EXCEPTION, :data => { :message => { :b => 2 } }])
+      invoke_exception(:b => 2)
+      ::ExceptionNotifier::Notifier.verify
+    end
+  end
+
+  describe "with fake Exceptional" do
+    before do
+      ::Exceptional = Class.new do
+
+        def self.context(msg)
+          @msg = msg
+        end
+
+        def self.check_context
+          @msg
+        end
+      end
+
+      ::Exceptional::Config = MiniTest::Mock.new
+      ::Exceptional::Remote = MiniTest::Mock.new
+      ::Exceptional::ExceptionData = MiniTest::Mock.new
+    end
+
+    after do
+      Object.send(:remove_const, "Exceptional")
+    end
+
+    it "notifies Exceptional" do
+      ::Exceptional::Config.expect(:should_send_to_api?,true)
+      exception_data = MiniTest::Mock.new
+      ::Exceptional::Remote.expect(:error,nil,[exception_data])
+      ::Exceptional::ExceptionData.expect(:new,exception_data,[TEST_EXCEPTION])
+      invoke_exception(:c => 3)
+      assert_equal({:c => 3},::Exceptional.check_context,"did not record arguments properly")
+      ::Exceptional::Config.verify
+      ::Exceptional::Remote.verify
+      ::Exceptional::ExceptionData.verify
+    end
+  end
+end

test/test_middleware.rb

@@ -9,18 +9,6 @@ class TestMiddleware < MiniTest::Unit::TestCase
       Sidekiq.redis = REDIS
     end
 
-    it 'handles errors' do
-      handler = Sidekiq::Middleware::Server::ExceptionHandler.new
-
-      assert_raises ArgumentError do
-        handler.call('', { :a => 1 }, 'default') do
-          raise ArgumentError
-        end
-      end
-      assert_equal 1, $errors.size
-      assert_equal({ :a => 1 }, $errors[0][:parameters])
-    end
-
     class CustomMiddleware
       def initialize(name, recorder)
         @name = name
@@ -83,10 +71,3 @@ class TestMiddleware < MiniTest::Unit::TestCase
     end
   end
 end
-
-class FakeAirbrake
-  def self.notify(ex, hash)
-    $errors << hash
-  end
-end
-Airbrake = FakeAirbrake

test/test_processor.rb

@@ -2,31 +2,55 @@ require 'helper'
 require 'sidekiq/processor'
 
 class TestProcessor < MiniTest::Unit::TestCase
+  TestException = Class.new(StandardError)
+  TEST_EXCEPTION = TestException.new("kerboom!")
+
   describe 'with mock setup' do
     before do
       $invokes = 0
-      $errors = []
       @boss = MiniTest::Mock.new
+      @processor = ::Sidekiq::Processor.new(@boss)
       Celluloid.logger = nil
       Sidekiq.redis = REDIS
+      @exception_handler = MiniTest::Mock.new
     end
 
     class MockWorker
       include Sidekiq::Worker
       def perform(args)
-        raise "kerboom!" if args == 'boom'
+        raise TEST_EXCEPTION if args == 'boom'
         $invokes += 1
       end
     end
 
     it 'processes as expected' do
       msg = Sidekiq.dump_json({ 'class' => MockWorker.to_s, 'args' => ['myarg'] })
-      processor = ::Sidekiq::Processor.new(@boss)
-      @boss.expect(:processor_done!, nil, [processor])
-      processor.process(msg, 'default')
+      @boss.expect(:processor_done!, nil, [@processor])
+      @processor.process(msg, 'default')
       @boss.verify
       assert_equal 1, $invokes
-      assert_equal 0, $errors.size
+    end
+
+    it 'passes exceptions to ExceptionHandler' do
+      msg = Sidekiq.dump_json({ 'class' => MockWorker.to_s, 'args' => ['boom'] })
+      @processor.exception_handler = @exception_handler
+      @exception_handler.expect(:handle,nil,[TEST_EXCEPTION,{"class"=>"TestProcessor::MockWorker", "args"=>["boom"]}])
+      @processor.process(msg, 'default') rescue TestException
+      @exception_handler.verify
+      assert_equal 0, $invokes
+    end
+
+    it 're-raises exceptions after handling' do
+      msg = Sidekiq.dump_json({ 'class' => MockWorker.to_s, 'args' => ['boom'] })
+      re_raise = false
+
+      begin
+        @processor.process(msg, 'default')
+      rescue TestException
+        re_raise = true
+      end
+
+      assert re_raise, "does not re-raise exceptions after handling"
     end
   end
 end