Merge pull request #658 from womblep/issue656

issue656 - change recoverable_network_failure?

lib/bunny/session.rb

@@ -71,6 +71,7 @@ class Session
     # Default reconnection interval for TCP connection failures
     DEFAULT_NETWORK_RECOVERY_INTERVAL = 5.0
 
+    DEFAULT_RECOVERABLE_EXCEPTIONS = [StandardError, TCPConnectionFailedForAllHosts, TCPConnectionFailed, AMQ::Protocol::EmptyResponseError, SystemCallError, Timeout::Error, Bunny::ConnectionLevelException, Bunny::ConnectionClosedError]
 
     #
     # API
@@ -91,6 +92,7 @@ class Session
     attr_reader :network_recovery_interval
     attr_reader :connection_name
     attr_accessor :socket_configurator
+    attr_accessor :recoverable_exceptions
 
     # @param [String, Hash] connection_string_or_opts Connection string or a hash of connection options
     # @param [Hash] optz Extra options not related to connection
@@ -226,6 +228,8 @@ def initialize(connection_string_or_opts = ENV['RABBITMQ_URL'], optz = Hash.new)
 
       @session_error_handler = opts.fetch(:session_error_handler, Thread.current)
 
+      @recoverable_exceptions = DEFAULT_RECOVERABLE_EXCEPTIONS.dup
+
       self.reset_continuations
       self.initialize_transport
 
@@ -747,9 +751,7 @@ def handle_network_failure(exception)
 
     # @private
     def recoverable_network_failure?(exception)
-      # No reasonably smart strategy was suggested in a few years.
-      # So just recover unconditionally. MK.
-      true
+      @recoverable_exceptions.any? {|x| exception.kind_of? x}
     end
 
     # @private
@@ -794,19 +796,22 @@ def recover_from_network_failure
     rescue HostListDepleted
       reset_address_index
       retry
-    rescue TCPConnectionFailedForAllHosts, TCPConnectionFailed, AMQ::Protocol::EmptyResponseError, SystemCallError, Timeout::Error => e
-      @logger.warn "TCP connection failed, reconnecting in #{@network_recovery_interval} seconds"
-      if should_retry_recovery?
-        decrement_recovery_attemp_counter!
-        if recoverable_network_failure?(e)
+    rescue => e
+      if recoverable_network_failure?(e)
+        @logger.warn "TCP connection failed"
+        if should_retry_recovery?
+          @logger.warn "Reconnecting in #{@network_recovery_interval} seconds"
+          decrement_recovery_attemp_counter!
           announce_network_failure_recovery
           retry
+        else
+          @logger.error "Ran out of recovery attempts (limit set to #{@max_recovery_attempts}), giving up"
+          @transport.close
+          self.close(false)
+          @manually_closed = false
         end
       else
-        @logger.error "Ran out of recovery attempts (limit set to #{@max_recovery_attempts}), giving up"
-        @transport.close
-        self.close(false)
-        @manually_closed = false
+        raise e
       end
     end
 
@@ -1356,7 +1361,7 @@ def initialize_transport
                                    host_from_address(address),
                                    port_from_address(address),
                                    @opts.merge(:session_error_handler => @session_error_handler)
-                                  )
+        )
 
         # Reset the cached progname for the logger only when no logger was provided
         @default_logger.progname = self.to_s

spec/issues/issue656_spec.rb

@@ -0,0 +1,61 @@
+require 'spec_helper'
+
+describe Bunny::Session do
+  context 'when created' do
+    let(:io) { StringIO.new } # keep test output clear
+
+    def create_session
+      described_class.new(
+        host: 'fake.host',
+        recovery_attempts: 0,
+        connection_timeout: 0,
+        network_recovery_interval: 0,
+        logfile: io,
+        )
+    end
+
+    it 'has default exceptions' do
+      session = create_session
+      expect(session.recoverable_exceptions).to eq(Bunny::Session::DEFAULT_RECOVERABLE_EXCEPTIONS)
+    end
+
+    it 'can override exceptions' do
+      session = create_session
+      session.recoverable_exceptions = [StandardError]
+      expect(session.recoverable_exceptions).to eq([StandardError])
+    end
+
+    it 'can append exceptions' do
+      session = create_session
+      session.recoverable_exceptions << StandardError
+      expect(session.recoverable_exceptions).to eq(Bunny::Session::DEFAULT_RECOVERABLE_EXCEPTIONS.append(StandardError))
+    end
+  end
+
+  context 'when retry attempts have been exhausted' do
+    let(:io) { StringIO.new } # keep test output clear
+
+    def create_session
+      described_class.new(
+        host: 'fake.host',
+        recovery_attempts: 0,
+        connection_timeout: 0,
+        network_recovery_interval: 0,
+        logfile: io,
+      )
+    end
+
+    it 'closes the session' do
+      session = create_session
+      session.handle_network_failure(StandardError.new)
+      expect(session.closed?).to be true
+    end
+
+    it 'stops the reader loop' do
+      session = create_session
+      reader_loop = session.reader_loop
+      session.handle_network_failure(StandardError.new)
+      expect(reader_loop.stopping?).to be true
+    end
+  end
+end