Merge pull request #224 from javanthropus/dry_up_connection_handling

DRY up connection handling logic

Author: jch

lib/net/ldap/connection.rb

@@ -8,56 +8,43 @@ 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?
       open_connection(server)
     end
 
     yield self if block_given?
   end
 
   def prepare_socket(server)
-    @conn = server[:socket]
+    socket = server[:socket]
+    encryption = server[:encryption]
 
-    if server[:encryption]
-      setup_encryption server[:encryption]
-    end
+    @conn = socket
+    setup_encryption encryption if encryption
   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)))
+        return
+      rescue Net::LDAP::Error, SocketError, SystemCallError,
+             OpenSSL::SSL::SSLError => e
+        # Ensure the connection is closed in the event a setup failure.
+        close
+        errors << [e, host, port]
       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
@@ -156,6 +143,7 @@ def setup_encryption(args)
   # have to call it, but perhaps it will come in handy someday.
   #++
   def close
+    return if @conn.nil?
     @conn.close
     @conn = nil
   end

lib/net/ldap/error.rb

@@ -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

test/test_ldap_connection.rb

@@ -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
@@ -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