Merge pull request #235 from ruby-ldap/lazy-socket-initialization

Lazy initialize Net::LDAP::Connection's internal socket

Author: jch

Diff for: lib/net/ldap.rb

@@ -1275,6 +1275,11 @@ def inspect
     inspected
   end
 
+  # Internal: Set @open_connection for testing
+  def connection=(connection)
+    @open_connection = connection
+  end
+
   private
 
   # Yields an open connection if there is one, otherwise establishes a new
@@ -1300,13 +1305,17 @@ def use_connection(args)
 
   # Establish a new connection to the LDAP server
   def new_connection
-    Net::LDAP::Connection.new \
+    connection = Net::LDAP::Connection.new \
       :host                    => @host,
       :port                    => @port,
       :hosts                   => @hosts,
       :encryption              => @encryption,
       :instrumentation_service => @instrumentation_service,
       :connect_timeout         => @connect_timeout
+
+    # Force connect to see if there's a connection error
+    connection.socket
+    connection
   rescue Errno::ECONNREFUSED, Errno::ETIMEDOUT, Net::LDAP::ConnectionRefusedError => e
     @result = {
       :resultCode   => 52,

Diff for: lib/net/ldap/connection.rb

@@ -9,19 +9,28 @@ class Net::LDAP::Connection #:nodoc:
   LdapVersion = 3
   MaxSaslChallenges = 10
 
-  def initialize(server)
+  # Initialize a connection to an LDAP server
+  #
+  # :server
+  #   :hosts   Array of tuples specifying host, port
+  #   :host    host
+  #   :port    port
+  #   :socket  prepared socket
+  #
+  def initialize(server = {})
+    @server = server
     @instrumentation_service = server[:instrumentation_service]
 
-    if server[:socket]
-      prepare_socket(server)
-    else
-      server[:hosts] = [[server[:host], server[:port]]] if server[:hosts].nil?
-      open_connection(server)
-    end
+    # Allows tests to parameterize what socket class to use
+    @socket_class = server.fetch(:socket_class, DefaultSocket)
 
     yield self if block_given?
   end
 
+  def socket_class=(socket_class)
+    @socket_class = socket_class
+  end
+
   def prepare_socket(server)
     socket = server[:socket]
     encryption = server[:encryption]
@@ -41,7 +50,7 @@ def open_connection(server)
     errors = []
     hosts.each do |host, port|
       begin
-        prepare_socket(server.merge(socket: Socket.tcp(host, port, socket_opts)))
+        prepare_socket(server.merge(socket: @socket_class.new(host, port, socket_opts)))
         return
       rescue Net::LDAP::Error, SocketError, SystemCallError,
              OpenSSL::SSL::SSLError => e
@@ -202,7 +211,7 @@ def message_queue
   def read(syntax = Net::LDAP::AsnSyntax)
     ber_object =
       instrument "read.net_ldap_connection", :syntax => syntax do |payload|
-        @conn.read_ber(syntax) do |id, content_length|
+        socket.read_ber(syntax) do |id, content_length|
           payload[:object_type_id] = id
           payload[:content_length] = content_length
         end
@@ -232,7 +241,7 @@ def read(syntax = Net::LDAP::AsnSyntax)
   def write(request, controls = nil, message_id = next_msgid)
     instrument "write.net_ldap_connection" do |payload|
       packet = [message_id.to_ber, request, controls].compact.to_ber_sequence
-      payload[:content_length] = @conn.write(packet)
+      payload[:content_length] = socket.write(packet)
     end
   end
   private :write
@@ -652,4 +661,33 @@ def delete(args)
 
     pdu
   end
+
+  # Internal: Returns a Socket like object used internally to communicate with
+  # LDAP server.
+  #
+  # Typically a TCPSocket, but can be a OpenSSL::SSL::SSLSocket
+  def socket
+    return @conn if defined? @conn
+
+    # First refactoring uses the existing methods open_connection and
+    # prepare_socket to set @conn. Next cleanup would centralize connection
+    # handling here.
+    if @server[:socket]
+      prepare_socket(@server)
+    else
+      @server[:hosts] = [[@server[:host], @server[:port]]] if @server[:hosts].nil?
+      open_connection(@server)
+    end
+
+    @conn
+  end
+
+  private
+
+  # Wrap around Socket.tcp to normalize with other Socket initializers
+  class DefaultSocket
+    def self.new(host, port, socket_opts = {})
+      Socket.tcp(host, port, socket_opts)
+    end
+  end
 end # class Connection

Diff for: test/test_auth_adapter.rb

@@ -1,10 +1,13 @@
 require 'test_helper'
 
 class TestAuthAdapter < Test::Unit::TestCase
-  def test_undefined_auth_adapter
-    flexmock(Socket).should_receive(:tcp).ordered.with('ldap.example.com', 379, { connect_timeout: 5 }).once.and_return(nil)
+  class FakeSocket
+    def initialize(*args)
+    end
+  end
 
-    conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379)
+  def test_undefined_auth_adapter
+    conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379, :socket_class => FakeSocket)
     assert_raise Net::LDAP::AuthMethodUnsupportedError, "Unsupported auth method (foo)" do
       conn.bind(method: :foo)
     end

Diff for: test/test_ldap.rb

@@ -1,6 +1,28 @@
 require 'test_helper'
 
 class TestLDAPInstrumentation < Test::Unit::TestCase
+  # Fake Net::LDAP::Connection for testing
+  class FakeConnection
+    # It's difficult to instantiate Net::LDAP::PDU objects. Faking out what we
+    # need here until that object is brought under test and has it's constructor
+    # cleaned up.
+    class Result < Struct.new(:success?, :result_code); end
+
+    def initialize
+      @bind_success = Result.new(true, Net::LDAP::ResultCodeSuccess)
+      @search_success = Result.new(true, Net::LDAP::ResultCodeSizeLimitExceeded)
+    end
+
+    def bind(args = {})
+      @bind_success
+    end
+
+    def search(*args)
+      yield @search_success if block_given?
+      @search_success
+    end
+  end
+
   def setup
     @connection = flexmock(:connection, :close => true)
     flexmock(Net::LDAP::Connection).should_receive(:new).and_return(@connection)
@@ -15,8 +37,9 @@ def setup
   def test_instrument_bind
     events = @service.subscribe "bind.net_ldap"
 
-    bind_result = flexmock(:bind_result, :success? => true)
-    flexmock(@connection).should_receive(:bind).with(Hash).and_return(bind_result)
+    fake_connection = FakeConnection.new
+    @subject.connection = fake_connection
+    bind_result = fake_connection.bind
 
     assert @subject.bind
 
@@ -28,10 +51,9 @@ def test_instrument_bind
   def test_instrument_search
     events = @service.subscribe "search.net_ldap"
 
-    flexmock(@connection).should_receive(:bind).and_return(flexmock(:bind_result, :result_code => Net::LDAP::ResultCodeSuccess))
-    flexmock(@connection).should_receive(:search).with(Hash, Proc).
-                yields(entry = Net::LDAP::Entry.new("uid=user1,ou=users,dc=example,dc=com")).
-                and_return(flexmock(:search_result, :success? => true, :result_code => Net::LDAP::ResultCodeSuccess))
+    fake_connection = FakeConnection.new
+    @subject.connection = fake_connection
+    entry = fake_connection.search
 
     refute_nil @subject.search(:filter => "(uid=user1)")
 
@@ -44,10 +66,9 @@ def test_instrument_search
   def test_instrument_search_with_size
     events = @service.subscribe "search.net_ldap"
 
-    flexmock(@connection).should_receive(:bind).and_return(flexmock(:bind_result, :result_code => Net::LDAP::ResultCodeSuccess))
-    flexmock(@connection).should_receive(:search).with(Hash, Proc).
-                yields(entry = Net::LDAP::Entry.new("uid=user1,ou=users,dc=example,dc=com")).
-                and_return(flexmock(:search_result, :success? => true, :result_code => Net::LDAP::ResultCodeSizeLimitExceeded))
+    fake_connection = FakeConnection.new
+    @subject.connection = fake_connection
+    entry = fake_connection.search
 
     refute_nil @subject.search(:filter => "(uid=user1)", :size => 1)
 

Diff for: test/test_ldap_connection.rb

@@ -9,44 +9,55 @@ def capture_stderr
     $stderr = stderr
   end
 
+  # Fake socket for testing
+  #
+  # FakeTCPSocket.new("success", 636)
+  # FakeTCPSocket.new("fail.SocketError", 636)  # raises SocketError
+  class FakeTCPSocket
+    def initialize(host, port, socket_opts = {})
+      status, error = host.split(".")
+      if status == "fail"
+        raise Object.const_get(error)
+      end
+    end
+  end
+
   def test_list_of_hosts_with_first_host_successful
     hosts = [
-      ['test.mocked.com', 636],
-      ['test2.mocked.com', 636],
-      ['test3.mocked.com', 636],
+      ["success.host", 636],
+      ["fail.SocketError", 636],
+      ["fail.SocketError", 636],
     ]
-    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_return(nil)
-    flexmock(Socket).should_receive(:tcp).ordered.never
-    Net::LDAP::Connection.new(:hosts => hosts)
+
+    connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket)
+    connection.socket
   end
 
   def test_list_of_hosts_with_first_host_failure
     hosts = [
-      ['test.mocked.com', 636],
-      ['test2.mocked.com', 636],
-      ['test3.mocked.com', 636],
+      ["fail.SocketError", 636],
+      ["success.host", 636],
+      ["fail.SocketError", 636],
     ]
-    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_raise(SocketError)
-    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[1], { connect_timeout: 5 }).once.and_return(nil)
-    flexmock(Socket).should_receive(:tcp).ordered.never
-    Net::LDAP::Connection.new(:hosts => hosts)
+
+    connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket)
+    connection.socket
   end
 
   def test_list_of_hosts_with_all_hosts_failure
     hosts = [
-      ['test.mocked.com', 636],
-      ['test2.mocked.com', 636],
-      ['test3.mocked.com', 636],
+      ["fail.SocketError", 636],
+      ["fail.SocketError", 636],
+      ["fail.SocketError", 636],
     ]
-    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_raise(SocketError)
-    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[1], { connect_timeout: 5 }).once.and_raise(SocketError)
-    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[2], { connect_timeout: 5 }).once.and_raise(SocketError)
-    flexmock(Socket).should_receive(:tcp).ordered.never
+
+    connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket)
     assert_raise Net::LDAP::ConnectionError do
-      Net::LDAP::Connection.new(:hosts => hosts)
+      connection.socket
     end
   end
 
+  # This belongs in test_ldap, not test_ldap_connection
   def test_result_for_connection_failed_is_set
     flexmock(Socket).should_receive(:tcp).and_raise(Errno::ECONNREFUSED)
 
@@ -61,42 +72,42 @@ def test_result_for_connection_failed_is_set
   end
 
   def test_unresponsive_host
+    connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636, :socket_class => FakeTCPSocket)
     assert_raise Net::LDAP::Error do
-      Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
+      connection.socket
     end
   end
 
   def test_blocked_port
-    flexmock(Socket).should_receive(:tcp).and_raise(SocketError)
+    connection = Net::LDAP::Connection.new(:host => "fail.SocketError", :port => 636, :socket_class => FakeTCPSocket)
     assert_raise Net::LDAP::Error do
-      Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
+      connection.socket
     end
   end
 
   def test_connection_refused
-    flexmock(Socket).should_receive(:tcp).and_raise(Errno::ECONNREFUSED)
+    connection = Net::LDAP::Connection.new(:host => "fail.Errno::ECONNREFUSED", :port => 636, :socket_class => FakeTCPSocket)
     stderr = capture_stderr do
       assert_raise Net::LDAP::ConnectionRefusedError do
-        Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
+        connection.socket
       end
     end
     assert_equal("Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead.\n",  stderr)
   end
 
-  def test_connection_timedout
-    flexmock(Socket).should_receive(:tcp).and_raise(Errno::ETIMEDOUT)
+  def test_connection_timeout
+    connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636, :socket_class => FakeTCPSocket)
     stderr = capture_stderr do
       assert_raise Net::LDAP::Error do
-        Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
+        connection.socket
       end
     end
   end
 
   def test_raises_unknown_exceptions
-    error = Class.new(StandardError)
-    flexmock(Socket).should_receive(:tcp).and_raise(error)
-    assert_raise error do
-      Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
+    connection = Net::LDAP::Connection.new(:host => "fail.StandardError", :port => 636, :socket_class => FakeTCPSocket)
+    assert_raise StandardError do
+      connection.socket
     end
   end