Merge pull request #1294 from appsignal/ignore-puma-connection-error

Ignore Errno::EPIPE error causes when reading body (Puma)

.changesets/do-not-report-puma--connectionerror-when-instrumenting-response-bodies.md

@@ -0,0 +1,6 @@
+---
+bump: patch
+type: change
+---
+
+Do not report errors caused by `Errno::EPIPE` (broken pipe errors) when instrumenting response bodies, to avoid reporting errors that cannot be fixed by the application.

lib/appsignal/rack/body_wrapper.rb

@@ -54,7 +54,7 @@ def close
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
 
@@ -72,6 +72,19 @@ def method_missing(method_name, *args, &block)
         @body.__send__(method_name, *args, &block)
       end
       ruby2_keywords(:method_missing) if respond_to?(:ruby2_keywords, true)
+
+      private
+
+      def appsignal_report_error(error)
+        @transaction.set_error(error) if appsignal_accepted_error?(error)
+      end
+
+      def appsignal_accepted_error?(error)
+        return true unless error.cause
+        return false if IGNORED_ERRORS.include?(error.cause.class)
+
+        appsignal_accepted_error?(error.cause)
+      end
     end
 
     # The standard Rack body wrapper which exposes "each" for iterating
@@ -97,7 +110,7 @@ def each(&blk)
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
     end
@@ -118,7 +131,7 @@ def call(stream)
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
     end
@@ -144,7 +157,7 @@ def to_ary
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
     end
@@ -162,7 +175,7 @@ def to_path
       rescue *IGNORED_ERRORS # Do not report
         raise
       rescue Exception => error # rubocop:disable Lint/RescueException
-        @transaction.set_error(error)
+        appsignal_report_error(error)
         raise error
       end
     end

spec/lib/appsignal/rack/body_wrapper_spec.rb

@@ -90,6 +90,32 @@
       expect(transaction).to_not have_error
     end
 
+    it "does not report EPIPE error when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_body = double
+      expect(fake_body).to receive(:each).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        expect { |b| wrapped.each(&b) }.to yield_control
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
+    it "does not report EPIPE error when it's the nested error cause" do
+      error = error_with_nested_cause(StandardError, "error message", Errno::EPIPE)
+      fake_body = double
+      expect(fake_body).to receive(:each).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        expect { |b| wrapped.each(&b) }.to yield_control
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
     it "closes the body and tracks an instrumentation event when it gets closed" do
       fake_body = double(:close => nil)
       expect(fake_body).to receive(:each).once.and_yield("a").and_yield("b").and_yield("c")
@@ -100,6 +126,43 @@
 
       expect(transaction).to include_event("name" => "close_response_body.rack")
     end
+
+    it "reports an error if an error occurs on close" do
+      fake_body = double
+      expect(fake_body).to receive(:close).and_raise(ExampleException, "error message")
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        wrapped.close
+      end.to raise_error(ExampleException, "error message")
+
+      expect(transaction).to have_error("ExampleException", "error message")
+    end
+
+    it "doesn't report EPIPE error on close" do
+      fake_body = double
+      expect(fake_body).to receive(:close).and_raise(Errno::EPIPE)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        wrapped.close
+      end.to raise_error(Errno::EPIPE)
+
+      expect(transaction).to_not have_error
+    end
+
+    it "does not report EPIPE error when it's the error cause on close" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_body = double
+      expect(fake_body).to receive(:close).and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        wrapped.close
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
   end
 
   describe "with a body supporting both each() and call" do
@@ -165,6 +228,19 @@
       expect(transaction).to_not have_error
     end
 
+    it "does not report EPIPE error when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_body = double
+      expect(fake_body).to receive(:each).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        expect { |b| wrapped.each(&b) }.to yield_control
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
     it "reads out the body in full using to_ary" do
       expect(fake_body).to receive(:to_ary).and_return(["one", "two", "three"])
 
@@ -190,6 +266,21 @@
 
       expect(transaction).to have_error("ExampleException", "error message")
     end
+
+    it "does not report EPIPE error when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_body = double
+      allow(fake_body).to receive(:each)
+      expect(fake_body).to receive(:to_ary).once.and_raise(error)
+      expect(fake_body).to_not receive(:close) # Per spec we expect the body has closed itself
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        wrapped.to_ary
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
   end
 
   describe "with a body supporting both to_path and each" do
@@ -249,6 +340,30 @@
       expect(transaction).to_not have_error
     end
 
+    it "does not report EPIPE error from #each when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      expect(fake_body).to receive(:each).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        expect { |b| wrapped.each(&b) }.to yield_control
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
+    it "does not report EPIPE error from #to_path when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      allow(fake_body).to receive(:to_path).once.and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+      expect do
+        wrapped.to_path
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+
     it "exposes to_path to the sender" do
       allow(fake_body).to receive(:to_path).and_return("/tmp/file.bin")
 
@@ -315,5 +430,45 @@
 
       expect(transaction).to_not have_error
     end
+
+    it "does not report EPIPE error from #call when it's the error cause" do
+      error = error_with_cause(StandardError, "error message", Errno::EPIPE)
+      fake_rack_stream = double
+      allow(fake_body).to receive(:call)
+        .with(fake_rack_stream)
+        .and_raise(error)
+
+      wrapped = described_class.wrap(fake_body, transaction)
+
+      expect do
+        wrapped.call(fake_rack_stream)
+      end.to raise_error(StandardError, "error message")
+
+      expect(transaction).to_not have_error
+    end
+  end
+
+  def error_with_cause(klass, message, cause)
+    begin
+      raise cause
+    rescue
+      raise klass, message
+    end
+  rescue => error
+    error
+  end
+
+  def error_with_nested_cause(klass, message, cause)
+    begin
+      begin
+        raise cause
+      rescue
+        raise klass, message
+      end
+    rescue
+      raise klass, message
+    end
+  rescue => error
+    error
   end
 end