Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed issue with reconnect mechanism #251

Merged
merged 2 commits into from Sep 30, 2014
Merged

Fixed issue with reconnect mechanism #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac9bcce
Fixed issue with reconnect mechanism
foeken Sep 30, 2014
fdc915a
Changed the wording of the empty frame exception
foeken Sep 30, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/bunny/exceptions.rb
View file
Open in desktop
Expand Up @@ -65,7 +65,7 @@ def initialize(message, connection, connection_close = nil)
class TCPConnectionFailed < Exception
attr_reader :hostname, :port

def initialize(e, hostname, port)
def initialize(e, hostname=nil, port=nil)
m = case e
when String then
e
Expand Down
14 changes: 5 additions & 9 deletions lib/bunny/session.rb
View file
Open in desktop
Expand Up @@ -285,7 +285,7 @@ def start
@logger.warn "Retrying connection on next host in line: #{@transport.host}:#{@transport.port}"

return self.start
rescue Exception
rescue
@status_mutex.synchronize { @status = :not_connected }
raise
end
Expand Down Expand Up @@ -961,13 +961,11 @@ def open_connection
fr
# frame timeout means the broker has closed the TCP connection, which it
# does per 0.9.1 spec.
rescue Errno::ECONNRESET, ClientTimeout, AMQ::Protocol::EmptyResponseError, EOFError, IOError => e
rescue
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to be careful here and catch everything. We could add some logging though.

nil
end
if frame.nil?
@state = :closed
@logger.error "RabbitMQ closed TCP connection before AMQP 0.9.1 connection was finalized. Most likely this means authentication failure."
raise Bunny::PossibleAuthenticationFailureError.new(self.user, self.vhost, self.password.size)
raise TCPConnectionFailed.new('An empty frame was received while opening the connection. In RabbitMQ <= 3.1 this could mean an authentication issue.')
end

response = frame.decode_payload
Expand Down Expand Up @@ -1011,13 +1009,11 @@ def open_connection
fr
# frame timeout means the broker has closed the TCP connection, which it
# does per 0.9.1 spec.
rescue Errno::ECONNRESET, ClientTimeout, AMQ::Protocol::EmptyResponseError, EOFError => e
rescue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intent here? Rescue everything? Then it should be rescue Exception => e. Bare rescue is the same as rescue StandardError => e.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would StandardError not suffice here? Is there any reasons we should catch the base Exception class in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looks like it's fine.

nil
end
if frame2.nil?
@state = :closed
@logger.warn "RabbitMQ closed TCP connection before AMQP 0.9.1 connection was finalized. Most likely this means authentication failure."
raise Bunny::PossibleAuthenticationFailureError.new(self.user, self.vhost, self.password.size)
raise TCPConnectionFailed.new('An empty frame was received while opening the connection. In RabbitMQ <= 3.1 this could mean an authentication issue.')
end
connection_open_ok = frame2.decode_payload

Expand Down