Merge pull request #282 from rollbar/reraise-on-async-processing

Reraise exceptions on Rollbar.process_payload_safely.

README.md

@@ -518,11 +518,11 @@ You can supply your own handler using ```config.async_handler```. The object to
 ```ruby
 config.use_async = true
 config.async_handler = Proc.new { |payload|
-  Thread.new { Rollbar.process_payload_safely(payload) }
+  Thread.new { Rollbar.process_from_async_handler(payload) }
 }
 ```
 
-Make sure you pass ```payload``` to ```Rollbar.process_payload_safely``` in your own implementation.
+Make sure you pass ```payload``` to ```Rollbar.process_from_async_handler``` in your own implementation.
 
 ## Failover handlers
 

lib/generators/rollbar/templates/initializer.rb

@@ -47,7 +47,7 @@
   # config.use_async = true
   # Supply your own async handler:
   # config.async_handler = Proc.new { |payload|
-  #  Thread.new { Rollbar.process_payload_safely(payload) }
+  #  Thread.new { Rollbar.process_from_async_handler(payload) }
   # }
 
   # Enable asynchronous reporting (using sucker_punch)

lib/rollbar.rb

@@ -17,7 +17,7 @@
 require 'rollbar/exception_reporter'
 require 'rollbar/util'
 require 'rollbar/railtie' if defined?(Rails::VERSION)
-require 'rollbar/delay/girl_friday'
+require 'rollbar/delay/girl_friday' if defined?(GirlFriday)
 require 'rollbar/delay/thread'
 require 'rollbar/truncation'
 
@@ -27,7 +27,7 @@ module Rollbar
     Rack::Multipart::UploadedFile
   ].freeze
   PUBLIC_NOTIFIER_METHODS = %w(debug info warn warning error critical log logger
-                               process_payload process_payload_safely scope send_failsafe log_info log_debug
+                               process_payload process_from_async_handler scope send_failsafe log_info log_debug
                                log_warning log_error silenced)
 
   class Notifier
@@ -195,10 +195,39 @@ def process_payload(payload)
       raise e
     end
 
-    def process_payload_safely(payload)
-      process_payload(payload)
-    rescue => e
-      report_internal_error(e)
+    # We will reraise exceptions in this method so async queues
+    # can retry the job or, in general, handle an error report some way.
+    #
+    # At same time that exception is silenced so we don't generate
+    # infinite reports. This example is what we want to avoid:
+    #
+    # 1. New exception in a the project is raised
+    # 2. That report enqueued to Sidekiq queue.
+    # 3. The Sidekiq job tries to send the report to our API
+    # 4. The report fails, for example cause a network failure,
+    #    and a exception is raised
+    # 5. We report an internal error for that exception
+    # 6. We reraise the exception so Sidekiq job fails and
+    #    Sidekiq can retry the job reporting the original exception
+    # 7. Because the job failed and Sidekiq can be managed by rollbar we'll
+    #    report a new exception.
+    # 8. Go to point 2.
+    #
+    # We'll then push to Sidekiq queue indefinitely until the network failure
+    # is fixed.
+    #
+    # Using Rollbar.silenced we avoid the above behavior but Sidekiq
+    # will have a chance to retry the original job.
+    def process_from_async_handler(payload)
+      Rollbar.silenced do
+        begin
+          process_payload(payload)
+        rescue => e
+          report_internal_error(e)
+
+          raise
+        end
+      end
     end
 
     def custom_data

lib/rollbar/delay/girl_friday.rb

@@ -3,22 +3,28 @@ module Delay
     class GirlFriday
 
       class << self
-        attr_accessor :queue
+        def queue_class
+          ::GirlFriday::WorkQueue
+        end
 
         def call(payload)
           new.call(payload)
         end
-      end
 
-      def queue_class
-        ::GirlFriday::WorkQueue
+        def queue
+          @queue ||= self.queue_class.new(nil, :size => 5) do |payload|
+            begin
+              Rollbar.process_from_async_handler(payload)
+            rescue
+              # According to https://github.com/mperham/girl_friday/wiki#error-handling
+              # we reraise the exception so it can be handled some way
+              raise
+            end
+          end
+        end
       end
 
       def call(payload)
-        self.class.queue = queue_class.new(nil, :size => 5) do |payload|
-          Rollbar.process_payload_safely(payload)
-        end
-
         self.class.queue.push(payload)
       end
     end

lib/rollbar/delay/resque.rb

@@ -23,7 +23,12 @@ def self.perform(payload)
         end
 
         def perform(payload)
-          Rollbar.process_payload_safely(payload)
+          begin
+            Rollbar.process_from_async_handler(payload)
+          rescue
+            # Raise the exception so Resque can track the errored job
+            raise
+          end
         end
       end
     end

lib/rollbar/delay/sidekiq.rb

@@ -16,7 +16,13 @@ def call(payload)
       include ::Sidekiq::Worker
 
       def perform(*args)
-        Rollbar.process_payload_safely(*args)
+        begin
+          Rollbar.process_from_async_handler(*args)
+        rescue
+          # Raise the exception so Sidekiq can track the errored job
+          # and retry it
+          raise
+        end
       end
     end
   end

lib/rollbar/delay/sucker_punch.rb

@@ -11,7 +11,20 @@ def self.call(payload)
       end
 
       def perform(*args)
-        Rollbar.process_payload_safely(*args)
+        begin
+          Rollbar.process_from_async_handler(*args)
+        rescue
+          # SuckerPunch can configure an exception handler with:
+          #
+          # SuckerPunch.exception_handler { # do something here }
+          #
+          # This is just passed to Celluloid.exception_handler which will
+          # push the reiceved block to an array of handlers, by default empty, [].
+          #
+          # We reraise the exception here casue it's safe and users could have defined
+          # their own exception handler for SuckerPunch
+          raise
+        end
       end
     end
   end

lib/rollbar/delay/thread.rb

@@ -6,7 +6,18 @@ def self.call(payload)
       end
 
       def call(payload)
-        ::Thread.new { Rollbar.process_payload_safely(payload) }
+        ::Thread.new do
+          begin
+            Rollbar.process_from_async_handler(payload)
+          rescue
+            # Here we swallow the exception:
+            # 1. The original report wasn't sent.
+            # 2. An internal error was sent and logged
+            #
+            # If users want to handle this in some way they
+            # can provide a more custom Thread based implementation
+          end
+        end
       end
     end
   end

spec/delay/sidekiq_spec.rb

@@ -17,7 +17,7 @@ class Sidekiq
 
   describe "#perform" do
     it "performs payload" do
-      Rollbar.should_receive(:process_payload_safely).with(payload)
+      Rollbar.should_receive(:process_from_async_handler).with(payload)
       subject.perform payload
     end
   end
@@ -26,7 +26,7 @@ class Sidekiq
     shared_examples "a Rollbar processor" do
 
       it "processes payload" do
-        Rollbar.should_receive(:process_payload_safely).with(payload)
+        Rollbar.should_receive(:process_from_async_handler).with(payload)
 
         subject.call payload
         described_class.drain

spec/delay/sucker_punch_spec.rb

@@ -17,7 +17,7 @@ class SuckerPunch
     let(:payload) { "anything" }
 
     it "performs the task asynchronously" do
-      Rollbar.should_receive(:process_payload_safely)
+      Rollbar.should_receive(:process_from_async_handler)
 
       Rollbar::Delay::SuckerPunch.call payload
     end

spec/rollbar/delay/girl_friday_spec.rb

@@ -6,14 +6,36 @@
 require 'rollbar/delay/girl_friday'
 
 describe Rollbar::Delay::GirlFriday do
+  before do
+    queue_class = ::GirlFriday::WorkQueue.immediate!
+    allow(::Rollbar::Delay::GirlFriday).to receive(:queue_class).and_return(queue_class)
+  end
+
   describe '.call' do
     let(:payload) do
       { :key => 'value' }
     end
 
     it 'push the payload into the queue' do
-      expect_any_instance_of(::GirlFriday::WorkQueue).to receive(:push).with(payload)
+      expect(Rollbar).to receive(:process_from_async_handler).with(payload)
+
       described_class.call(payload)
     end
+
+    context 'with exceptions processing payload' do
+      let(:exception) { Exception.new }
+
+      before do
+        expect(Rollbar).to receive(:process_from_async_handler).with(payload).and_raise(exception)
+      end
+
+      it 'raises an exception cause we are using immediate queue' do
+        # This will not happen with a norma work queue cause this:
+        # https://github.com/mperham/girl_friday/blob/master/lib/girl_friday/work_queue.rb#L90-L106
+        expect do
+          described_class.call(payload)
+        end.to raise_error(exception)
+      end
+    end
   end
 end

spec/rollbar/delay/resque_spec.rb

@@ -7,15 +7,31 @@
       { :key => 'value' }
     end
 
+    let(:loaded_hash) do
+      Rollbar::JSON.load(Rollbar::JSON.dump(payload))
+    end
+
     before do
       allow(Resque).to receive(:inline?).and_return(true)
     end
 
     it 'process the payload' do
-      loaded_hash = Rollbar::JSON.load(Rollbar::JSON.dump(payload))
-
-      expect(Rollbar).to receive(:process_payload_safely).with(loaded_hash)
+      expect(Rollbar).to receive(:process_from_async_handler).with(loaded_hash)
       described_class.call(payload)
     end
+
+    context 'with exceptions processing payload' do
+      let(:exception) { Exception.new }
+
+      before do
+        expect(Rollbar).to receive(:process_from_async_handler).with(loaded_hash).and_raise(exception)
+      end
+
+      it 'raises an exception' do
+        expect do
+          described_class.call(payload)
+        end.to raise_error(exception)
+      end
+    end
   end
 end

spec/rollbar/delay/thread_spec.rb

@@ -5,10 +5,23 @@
     let(:payload) { { :key => 'value' } }
 
     it 'process the payload in a new thread' do
-      expect(Rollbar).to receive(:process_payload_safely).with(payload)
+      expect(Rollbar).to receive(:process_from_async_handler).with(payload)
 
-      th = described_class.call(payload)
-      th.join
+      described_class.call(payload).join
+    end
+
+    context 'with exceptions processing payload' do
+      let(:exception) { StandardError.new }
+
+      before do
+        expect(Rollbar).to receive(:process_from_async_handler).with(payload).and_raise(exception)
+      end
+
+      it 'doesnt raise any exception' do
+        expect do
+          described_class.call(payload).join
+        end.not_to raise_error(exception)
+      end
     end
   end
 end

spec/rollbar_spec.rb

@@ -1704,15 +1704,20 @@ class DummyClass
     end
   end
 
-  describe '.process_payload_safely' do
+  describe '.process_from_async_handler' do
     context 'with errors' do
       let(:exception) { StandardError.new('the error') }
 
-      it 'doesnt raise anything and sends internal error' do
+      it 'raises anything and sends internal error' do
         allow(Rollbar.notifier).to receive(:process_payload).and_raise(exception)
         expect(Rollbar.notifier).to receive(:report_internal_error).with(exception)
 
-        Rollbar.notifier.process_payload_safely({})
+        expect do
+          Rollbar.notifier.process_from_async_handler({})
+        end.to raise_error(exception)
+
+        rollbar_do_not_report = exception.instance_variable_get(:@_rollbar_do_not_report)
+        expect(rollbar_do_not_report).to be_eql(true)
       end
     end
   end