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.

Sign up for GitHub

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

DRY up connection handling logic #224

Merged
merged 6 commits into from
Oct 27, 2015
Merged

DRY up connection handling logic #224

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d5f7516
DRY up connection handling logic
Sep 28, 2015
edee2ee
Move connection error handling logic into a new error class
Oct 18, 2015
e1a0d13
Resolve rubocop violations
Oct 18, 2015
5a4ddb1
Assign exceptions to a variable rather than use $!
Oct 19, 2015
ab320af
Close the socket where opened when necessary
Oct 19, 2015
e829069
Move connection cleanup logic into the close method
Oct 26, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 22 additions & 31 deletions lib/net/ldap/connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,56 +8,47 @@ class Net::LDAP::Connection #:nodoc:

def initialize(server)
@instrumentation_service = server[:instrumentation_service]
server[:hosts] = [[server[:host], server[:port]]] if server[:hosts].nil?

if server[:socket]
prepare_socket(server)
else
server[:hosts] = [[server[:host], server[:port]]] if server[:hosts].nil?
Copy link
Member

Choose a reason for hiding this comment

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

Note: This works even when server[:host] and/or server[:port] are nil. It raises a SocketError and matches up with the old behavior.

open_connection(server)
end

yield self if block_given?
end

def prepare_socket(server)
@conn = server[:socket]
def prepare_socket(server, close = false)
socket = server[:socket]
encryption = server[:encryption]

if server[:encryption]
setup_encryption server[:encryption]
end
@conn = socket
setup_encryption encryption if encryption
rescue
# Ensure the connection is closed when requested in the event of an SSL
# setup failure.
@conn.close if close
Copy link
Member

Choose a reason for hiding this comment

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

This seems like new behavior. Why this change in this PR?

Copy link
Author

Choose a reason for hiding this comment

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

Sockets created by the library in open_connection need to be closed in the event that they cannot successfully be configured, such as for encryption. Otherwise, we leak file descriptors and hope that the GC releases them in a timely manner. Not good, especially in the multiple host case where we may fail to configure the sockets for numerous hosts in rapid succession.

This can also be handled within the open_connection method since that is the only place where we currently set the close parameter. Now that you bring it up, I think I like this option better, so I'll make this change.

@conn = nil
raise
end

def open_connection(server)
hosts = server[:hosts]
encryption = server[:encryption]

errors = []
server[:hosts].each do |host, port|
hosts.each do |host, port|
begin
return connect_to_host(host, port, server)
rescue Net::LDAP::Error
errors << $!
prepare_socket(server.merge(socket: TCPSocket.new(host, port)), true)
return
rescue Net::LDAP::Error, SocketError, SystemCallError,
OpenSSL::SSL::SSLError
errors << [$!, host, port]
Copy link
Member

Choose a reason for hiding this comment

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

Assign the error to a variable rather than using the implicit here:

rescue Net::LDAP::Error, SocketError, SystemCallError, OpenSSL::SSL::SSLError => e
  errors << [e, host, port]
end

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

end
end

raise errors.first if errors.size == 1
raise Net::LDAP::Error,
"Unable to connect to any given server: \n #{errors.join("\n ")}"
end

def connect_to_host(host, port, server)
begin
@conn = TCPSocket.new(host, port)
rescue SocketError
raise Net::LDAP::Error, "No such address or other socket error."
rescue Errno::ECONNREFUSED
raise Net::LDAP::ConnectionRefusedError, "Server #{host} refused connection on port #{port}."
rescue Errno::EHOSTUNREACH => error
raise Net::LDAP::Error, "Host #{host} was unreachable (#{error.message})"
rescue Errno::ETIMEDOUT
raise Net::LDAP::Error, "Connection to #{host} timed out."
end

if server[:encryption]
setup_encryption server[:encryption]
end
raise Net::LDAP::ConnectionError.new(errors)
end

module GetbyteForSSLSocket
Expand Down
19 changes: 19 additions & 0 deletions lib/net/ldap/error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,25 @@ def warn_deprecation_message
warn "Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead."
end
end
class ConnectionError < Error
def self.new(errors)
error = errors.first.first
if errors.size == 1
if error.kind_of? Errno::ECONNREFUSED
return Net::LDAP::ConnectionRefusedError.new(error.message)
end

return Net::LDAP::Error.new(error.message)
end

super
end

def initialize(errors)
message = "Unable to connect to any given server: \n #{errors.map { |e, h, p| "#{e.class}: #{e.message} (#{h}:#{p})" }.join("\n ")}"
super(message)
end
end
class NoOpenSSLError < Error; end
class NoStartTLSResultError < Error; end
class NoSearchBaseError < Error; end
Expand Down
26 changes: 13 additions & 13 deletions test/test_ldap_connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,21 @@ def capture_stderr

def test_list_of_hosts_with_first_host_successful
hosts = [
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
]
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
]
flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_return(nil)
flexmock(TCPSocket).should_receive(:new).ordered.never
Net::LDAP::Connection.new(:hosts => hosts)
end

def test_list_of_hosts_with_first_host_failure
hosts = [
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
]
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
]
flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_raise(SocketError)
flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_return(nil)
flexmock(TCPSocket).should_receive(:new).ordered.never
Expand All @@ -34,15 +34,15 @@ def test_list_of_hosts_with_first_host_failure

def test_list_of_hosts_with_all_hosts_failure
hosts = [
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
]
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
]
flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_raise(SocketError)
flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_raise(SocketError)
flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[2]).once.and_raise(SocketError)
flexmock(TCPSocket).should_receive(:new).ordered.never
assert_raise Net::LDAP::Error do
assert_raise Net::LDAP::ConnectionError do
Net::LDAP::Connection.new(:hosts => hosts)
end
end
Expand Down