Merge branch 'cloudamqp-bug_519'

ChangeLog.md

@@ -12,6 +12,19 @@ GitHub issue: [#541](https://github.com/ruby-amqp/bunny/issues/541).
 Contributed by Howard Ding.
 
 
+### Disabling Heartbeats Disables TCP Socket Read Timeouts
+
+Disabling heartbeats will also disable TCP socket read timeouts,
+since the two are effectively interconnected. In this case a mechanism
+such as [TCP keepalives](http://www.rabbitmq.com/heartbeats.html#tcp-keepalives) is assumed to be used.
+
+See [RabbitMQ heartbeats guide](http://www.rabbitmq.com/heartbeats.html) for a more
+detailed overview of the options.
+
+GH issue: [#519](https://github.com/ruby-amqp/bunny/issues/519).
+
+Contributed by Carl Hörberg.
+
 
 ## Changes between Bunny 2.8.0 and 2.9.0 (Jan 8th, 2018)
 

lib/bunny/session.rb

@@ -101,7 +101,7 @@ class Session
     # @option connection_string_or_opts [String] :username ("guest") Username
     # @option connection_string_or_opts [String] :password ("guest") Password
     # @option connection_string_or_opts [String] :vhost ("/") Virtual host to use
-    # @option connection_string_or_opts [Integer, Symbol] :heartbeat (:server) Heartbeat interval. :server means use the default suggested by RabbitMQ. 0 means no heartbeat (not recommended).
+    # @option connection_string_or_opts [Integer, Symbol] :heartbeat (:server) Heartbeat interval. :server means use the default suggested by RabbitMQ. 0 means heartbeats and socket read timeouts will be disabled (not recommended).
     # @option connection_string_or_opts [Integer] :network_recovery_interval (4) Recovery interval periodic network recovery will use. This includes initial pause after network failure.
     # @option connection_string_or_opts [Boolean] :tls (false) Should TLS/SSL be used?
     # @option connection_string_or_opts [String] :tls_cert (nil) Path to client TLS/SSL certificate file (.pem)
@@ -111,7 +111,7 @@ class Session
     # @option connection_string_or_opts [Symbol] :tls_version (negotiated) What TLS version should be used (:TLSv1, :TLSv1_1, or :TLSv1_2)
     # @option connection_string_or_opts [Integer] :continuation_timeout (15000) Timeout for client operations that expect a response (e.g. {Bunny::Queue#get}), in milliseconds.
     # @option connection_string_or_opts [Integer] :connection_timeout (30) Timeout in seconds for connecting to the server.
-    # @option connection_string_or_opts [Integer] :read_timeout (30) TCP socket read timeout in seconds.
+    # @option connection_string_or_opts [Integer] :read_timeout (30) TCP socket read timeout in seconds. If heartbeats are disabled this will be ignored.
     # @option connection_string_or_opts [Integer] :write_timeout (30) TCP socket write timeout in seconds.
     # @option connection_string_or_opts [Proc] :hosts_shuffle_strategy A Proc that reorders a list of host strings, defaults to Array#shuffle
     # @option connection_string_or_opts [Logger] :logger The logger.  If missing, one is created using :log_file and :log_level.
@@ -1177,22 +1177,13 @@ def open_connection
       @logger.debug { "Heartbeat interval negotiation: client = #{@client_heartbeat}, server = #{connection_tune.heartbeat}, result = #{@heartbeat}" }
       @logger.info "Heartbeat interval used (in seconds): #{@heartbeat}"
 
-      # We set the read_write_timeout to twice the heartbeat value
+      # We set the read_write_timeout to twice the heartbeat value,
+      # and then some padding for edge cases.
       # This allows us to miss a single heartbeat before we time out the socket.
-      #
-      # Since RabbitMQ can be configured to disable heartbeats (bad idea but technically
-      # possible nonetheless), we need to take both client and server values into
-      # consideration when deciding about using the heartbeat value for read timeouts.
-      @transport.read_timeout = if heartbeat_disabled?(@client_heartbeat) || heartbeat_disabled?(@heartbeat)
-                                  @logger.debug { "Will use default socket read timeout of #{Transport::DEFAULT_READ_TIMEOUT}" }
-                                  Transport::DEFAULT_READ_TIMEOUT
-                                else
-                                  # pad to account for edge cases. MK.
-                                  n = @heartbeat * 2.2
-                                  @logger.debug { "Will use socket read timeout of #{n}" }
-                                  n
-                                end
-
+      # If heartbeats are disabled, assume that TCP keepalives or a similar mechanism will be used
+      # and disable socket read timeouts. See ruby-amqp/bunny#551.
+      @transport.read_timeout = @heartbeat * 2.2
+      @logger.debug { "Will use socket read timeout of #{@transport.read_timeout}" }
 
       # if there are existing channels we've just recovered from
       # a network failure and need to fix the allocated set. See issue 205. MK.

lib/bunny/transport.rb

@@ -29,6 +29,10 @@ class Transport
     attr_reader :tls_context, :verify_peer, :tls_ca_certificates, :tls_certificate_path, :tls_key_path
 
     attr_writer :read_timeout
+    def read_timeout=(v)
+      @read_timeout = v
+      @read_timeout = nil if @read_timeout == 0
+    end
 
     def initialize(session, host, port, opts)
       @session        = session