Fix RUBY-2144 Redundant socket connections during address resolution (#1769)

* shorten lines

* skip fewer tests on jruby

* test for number of sockets created

* get rid of the "resolver" step, return the created socket right away

* dry connection creation

* fix tests

Co-authored-by: Oleg Pudeyev <[email protected]>

Author: p-mongo

lib/mongo/address.rb

@@ -66,6 +66,8 @@ class Address
     # @param [ String ] seed The provided address.
     # @param [ Hash ] options The address options.
     #
+    # @option options [ Float ] :connect_timeout Connect timeout.
+    #
     # @since 2.0.0
     def initialize(seed, options = {})
       if seed.nil?
@@ -85,6 +87,9 @@ def initialize(seed, options = {})
     # @return [ Integer ] port The port.
     attr_reader :port
 
+    # @api private
+    attr_reader :options
+
     # Check equality of the address to another.
     #
     # @example Check address equality.
@@ -138,13 +143,30 @@ def inspect
       "#<Mongo::Address:0x#{object_id} address=#{to_s}>"
     end
 
-    # Get a socket for the provided address, given the options.
+    # Get a socket for the address stored in this object, given the options.
+    #
+    # If the address stored in this object looks like a Unix path, this method
+    # returns a Unix domain socket for this path.
     #
-    # The address the socket connects to is determined by the algorithm described in the
-    # #intialize_resolver! documentation. Each time this method is called, #initialize_resolver!
-    # will be called, meaning that a new hostname lookup will occur. This is done so that any
-    # changes to which addresses the hostname resolves to will be picked up even if a socket has
-    # been connected to it before.
+    # Otherwise, this method attempts to resolve the address stored in
+    # this object to IPv4 and IPv6 addresses using +Socket#getaddrinfo+, then
+    # connects to the resulting addresses and returns the socket of the first
+    # successful connection. The order in which address families (IPv4/IPV6)
+    # are tried is the same order in which the addresses are returned by
+    # +getaddrinfo+, and is determined by the host system.
+    #
+    # Name resolution is performed on each +socket+ call. This is done so that
+    # any changes to which addresses the host names used as seeds or in
+    # server configuration resolve to are immediately noticed by the driver,
+    # even if a socket has been connected to the affected host name/address
+    # before. However, note that DNS TTL values may still affect when a change
+    # to a host address is noticed by the driver.
+    #
+    # This method propagates any exceptions raised during DNS resolution and
+    # subsequent connection attempts. In case of a host name resolving to
+    # multiple IP addresses, the error raised by the last attempt is propagated
+    # to the caller. This method does not map exceptions to Mongo::Error
+    # subclasses, and may raise any subclass of Exception.
     #
     # @example Get a socket.
     #   address.socket(5, :ssl => true)
@@ -155,11 +177,34 @@ def inspect
     #
     # @option options [ Float ] :connect_timeout Connect timeout.
     #
-    # @return [ Mongo::Socket::SSL, Mongo::Socket::TCP, Mongo::Socket::Unix ] The socket.
+    # @return [ Mongo::Socket::SSL | Mongo::Socket::TCP | Mongo::Socket::Unix ]
+    #   The socket.
+    #
+    # @raise [ Exception ] If network connection failed.
     #
     # @since 2.0.0
     def socket(socket_timeout, ssl_options = {}, options = {})
-      create_resolver(ssl_options).socket(socket_timeout, ssl_options, options)
+      if seed.downcase =~ Unix::MATCH
+        specific_address = Unix.new(seed.downcase)
+        return specific_address.socket(socket_timeout, ssl_options, options)
+      end
+
+      options = {
+        connect_timeout: Server::CONNECT_TIMEOUT,
+      }.update(options)
+
+      family = (host == LOCALHOST) ? ::Socket::AF_INET : ::Socket::AF_UNSPEC
+      error = nil
+      ::Socket.getaddrinfo(host, nil, family, ::Socket::SOCK_STREAM).each do |info|
+        begin
+          specific_address = FAMILY_MAP[info[4]].new(info[3], port, host)
+          socket = specific_address.socket(socket_timeout, ssl_options, options)
+          return socket
+        rescue IOError, SystemCallError, Error::SocketTimeoutError, Error::SocketError => e
+          error = e
+        end
+      end
+      raise error
     end
 
     # Get the address as a string.
@@ -182,37 +227,8 @@ def to_s
       end
     end
 
-    # @api private
-    def connect_timeout
-      @connect_timeout ||= @options[:connect_timeout] || Server::CONNECT_TIMEOUT
-    end
-
     private
 
-    # To determine which address the socket will connect to, the driver will
-    # attempt to connect to each IP address returned by Socket::getaddrinfo in
-    # sequence. Once a successful connection is made, a resolver with that
-    # IP address specified is returned. If no successful connection is
-    # made, the error made by the last connection attempt is raised.
-    def create_resolver(ssl_options)
-      return Unix.new(seed.downcase) if seed.downcase =~ Unix::MATCH
-
-      family = (host == LOCALHOST) ? ::Socket::AF_INET : ::Socket::AF_UNSPEC
-      error = nil
-      ::Socket.getaddrinfo(host, nil, family, ::Socket::SOCK_STREAM).each do |info|
-        begin
-          specific_address = FAMILY_MAP[info[4]].new(info[3], port, host)
-          socket = specific_address.socket(
-            connect_timeout, ssl_options, connect_timeout: connect_timeout)
-          socket.close
-          return specific_address
-        rescue IOError, SystemCallError, Error::SocketTimeoutError, Error::SocketError => e
-          error = e
-        end
-      end
-      raise error
-    end
-
     def parse_host_port
       address = seed.downcase
       case address

lib/mongo/server/connection.rb

@@ -187,8 +187,7 @@ def connect!
 
       # Separate method to permit easier mocking in the test suite.
       def do_connect
-        socket = address.socket(socket_timeout, ssl_options,
-          connect_timeout: address.connect_timeout)
+        socket = address.socket(socket_timeout, ssl_options, address.options)
 
         begin
           handshake!(socket)

lib/mongo/server/monitor/connection.rb

@@ -162,8 +162,7 @@ def ismaster
         # @since 2.0.0
         def connect!
           unless @socket
-            socket = address.socket(socket_timeout, ssl_options,
-              connect_timeout: address.connect_timeout)
+            socket = address.socket(socket_timeout, ssl_options, address.options)
             handshake!(socket)
             @socket = socket
           end

spec/integration/connection_spec.rb

@@ -12,17 +12,18 @@
   let(:server) { client.cluster.servers.first }
 
   describe '#connect!' do
-    # On JRuby 9.2.7.0, this line:
-    # expect_any_instance_of(Mongo::Socket).to receive(:write).and_raise(exception)
-    # ... appears to produce a moment in which Mongo::Socket#write is undefined
-    # entirely, resulting in this failure:
-    # RSpec::Expectations::ExpectationNotMetError: expected Mongo::Error::SocketError, got #<NameError: undefined method `write' for class `Mongo::Socket'>
-    fails_on_jruby
+
+    let(:connection) do
+      Mongo::Server::Connection.new(server, server.options)
+    end
 
     context 'network error during handshake' do
-      let(:connection) do
-        Mongo::Server::Connection.new(server, server.options)
-      end
+      # On JRuby 9.2.7.0, this line:
+      # expect_any_instance_of(Mongo::Socket).to receive(:write).and_raise(exception)
+      # ... appears to produce a moment in which Mongo::Socket#write is undefined
+      # entirely, resulting in this failure:
+      # RSpec::Expectations::ExpectationNotMetError: expected Mongo::Error::SocketError, got #<NameError: undefined method `write' for class `Mongo::Socket'>
+      fails_on_jruby
 
       let(:exception) { Mongo::Error::SocketError }
 
@@ -120,6 +121,53 @@
           expect(client.cluster.topology).to be_a(Mongo::Cluster::Topology::ReplicaSetNoPrimary)
         end
       end
+
+      describe 'number of sockets created' do
+
+        before do
+          server
+        end
+
+        shared_examples_for 'is 1 per connection' do
+          it 'is 1 per connection' do
+            # Instantiating a connection object should not create any sockets
+            RSpec::Mocks.with_temporary_scope do
+              expect(socket_cls).not_to receive(:new)
+
+              connection
+            end
+
+            # When the connection connects, exactly one socket should be created
+            # (and subsequently connected)
+            RSpec::Mocks.with_temporary_scope do
+              expect(socket_cls).to receive(:new).and_call_original
+
+              connection.connect!
+            end
+          end
+        end
+
+        let(:socket_cls) { ::Socket }
+
+        it_behaves_like 'is 1 per connection'
+
+        context 'connection to Unix domain socket' do
+          # Server does not allow Unix socket connections when TLS is enabled
+          require_no_tls
+
+          let(:port) { SpecConfig.instance.any_port }
+
+          let(:client) do
+            new_local_client(["/tmp/mongodb-#{port}.sock"], connect: :direct).tap do |client|
+              stop_monitoring(client)
+            end
+          end
+
+          let(:socket_cls) { ::UNIXSocket }
+
+          it_behaves_like 'is 1 per connection'
+        end
+      end
     end
 
     describe 'wire protocol version range update' do

spec/mongo/address_spec.rb

@@ -249,11 +249,7 @@
       end
     end
 
-    context 'when creating a socket using the resolver' do
-
-      before do
-        address.send(:create_resolver, SpecConfig.instance.ssl_options)
-      end
+    context 'when creating a socket' do
 
       it 'uses the host, not the IP address' do
         expect(address.socket(0.0).host).to eq(socket_address_or_host)
@@ -285,6 +281,24 @@
         end
       end
     end
+
+    describe ':connect_timeout option' do
+      clean_slate
+
+      let(:address) { Mongo::Address.new('127.0.0.1') }
+
+      it 'defaults to 10' do
+        RSpec::Mocks.with_temporary_scope do
+          resolved_address = double('address')
+          # This test's expectation
+          expect(resolved_address).to receive(:socket).with(0, {}, connect_timeout: 10)
+
+          expect(Mongo::Address::IPv4).to receive(:new).and_return(resolved_address)
+
+          address.socket(0)
+        end
+      end
+    end
   end
 
   describe '#to_s' do
@@ -320,12 +334,4 @@
       end
     end
   end
-
-  describe '#connect_timeout' do
-    let(:address) { Mongo::Address.new('127.0.0.1') }
-
-    it 'defaults to 10' do
-      expect(address.send(:connect_timeout)).to eq(10)
-    end
-  end
 end

spec/mongo/server/connection_spec.rb

@@ -1080,7 +1080,7 @@ class ConnectionSpecTestException < Exception; end
         end
 
         it 'uses the connect_timeout for the address' do
-          expect(connection.address.send(:connect_timeout)).to eq(3)
+          expect(connection.address.options[:connect_timeout]).to eq(3)
         end
 
         it 'uses the socket_timeout as the socket_timeout' do
@@ -1099,7 +1099,7 @@ class ConnectionSpecTestException < Exception; end
         end
 
         it 'uses the connect_timeout for the address' do
-          expect(connection.address.send(:connect_timeout)).to eq(3)
+          expect(connection.address.options[:connect_timeout]).to eq(3)
         end
 
         it 'does not use a socket_timeout' do
@@ -1120,8 +1120,8 @@ class ConnectionSpecTestException < Exception; end
           connection.connect!
         end
 
-        it 'uses the default connect_timeout for the address' do
-          expect(connection.address.send(:connect_timeout)).to eq(10)
+        it 'does not specify connect_timeout for the address' do
+          expect(connection.address.options[:connect_timeout]).to be nil
         end
 
         it 'uses the socket_timeout' do
@@ -1139,8 +1139,8 @@ class ConnectionSpecTestException < Exception; end
           connection.connect!
         end
 
-        it 'uses the default connect_timeout for the address' do
-          expect(connection.address.send(:connect_timeout)).to eq(10)
+        it 'does not specify connect_timeout for the address' do
+          expect(connection.address.options[:connect_timeout]).to be nil
         end
 
         it 'does not use a socket_timeout' do

spec/mongo/server/monitor/connection_spec.rb

@@ -66,7 +66,7 @@
       end
 
       it 'uses the connect_timeout for the address' do
-        expect(connection.address.send(:connect_timeout)).to eq(3)
+        expect(connection.address.options[:connect_timeout]).to eq(3)
       end
 
       it 'uses the connect_timeout as the socket_timeout' do
@@ -81,7 +81,7 @@
       end
 
       it 'uses the connect_timeout for the address' do
-        expect(connection.address.send(:connect_timeout)).to eq(3)
+        expect(connection.address.options[:connect_timeout]).to eq(3)
       end
 
       it 'uses the connect_timeout as the socket_timeout' do
@@ -98,8 +98,8 @@
         SpecConfig.instance.test_options.merge(connect_timeout: nil, socket_timeout: 5)
       end
 
-      it 'uses the default connect_timeout for the address' do
-        expect(connection.address.send(:connect_timeout)).to eq(10)
+      it 'does not specify connect_timeout for the address' do
+        expect(connection.address.options[:connect_timeout]).to be nil
       end
 
       it 'uses the connect_timeout as the socket_timeout' do
@@ -113,8 +113,8 @@
         SpecConfig.instance.test_options.merge(connect_timeout: nil, socket_timeout: nil)
       end
 
-      it 'uses the default connect_timeout for the address' do
-        expect(connection.address.send(:connect_timeout)).to eq(10)
+      it 'does not specify connect_timeout for the address' do
+        expect(connection.address.options[:connect_timeout]).to be nil
       end
 
       it 'uses the connect_timeout as the socket_timeout' do