Backport RUBY-1894 Clear connection pools when monitor ismaster times out (#1475) (#1477)

This version always clears connection pools whenever a server becomes unknown.
This results in repeated pool clears sometimes which is suboptimal but not really wrong.

Driver version 2.11+ has the correct implementation of pool clearing which makes use
of refactoring done for background connection pool thread/SRV polling.

Author: p-mongo

lib/mongo/server.rb

@@ -417,7 +417,23 @@ def unknown!
 
     # @api private
     def update_description(description)
+      prev_description = monitor.instance_variable_get('@description')
       monitor.instance_variable_set('@description', description)
+      if description.unknown? && !prev_description.unknown?
+        # This clears redundantly sometimes and also clears the pool on
+        # 4.2+ servers after not master errors which should not be done.
+        # Driver version 2.11+ has the correct implementation.
+        clear_connection_pool
+      end
+    end
+
+    # @api private
+    def clear_connection_pool
+      @pool_lock.synchronize do
+        if @pool
+          @pool.disconnect!
+        end
+      end
     end
 
     # @api private

spec/integration/sdam_error_handling_spec.rb

@@ -5,80 +5,81 @@
     ClientRegistry.instance.close_all_clients
   end
 
-  describe 'when there is an error during an operation' do
+  # These tests operate on specific servers, and don't work in a multi
+  # shard cluster where multiple servers are equally eligible
+  require_no_multi_shard
 
-    # These tests operate on specific servers, and don't work in a multi
-    # shard cluster where multiple servers are equally eligible
-    require_no_multi_shard
+  let(:client) { authorized_client_without_any_retries }
 
-    let(:client) { authorized_client_without_any_retries }
+  let(:server) { client.cluster.next_primary }
 
-    before do
-      wait_for_all_servers(client.cluster)
-      # we also need a connection to the primary so that our error
-      # expectations do not get triggered during handshakes which
-      # have different behavior from non-handshake errors
-      client.database.command(ping: 1)
-      client.cluster.servers_list.each do |server|
-        server.monitor.stop!(true)
-      end
+  shared_examples_for 'marks server unknown' do
+    it 'marks server unknown' do
+      expect(server).not_to be_unknown
+      operation
+      expect(server).to be_unknown
     end
+  end
 
-    let(:server) { client.cluster.next_primary }
-
-    let(:operation) do
-      expect_any_instance_of(Mongo::Server::Connection).to receive(:deliver).and_return(reply)
-      expect do
-        client.database.command(ping: 1)
-      end.to raise_error(Mongo::Error::OperationFailure, exception_message)
+  shared_examples_for 'does not mark server unknown' do
+    it 'does not mark server unknown' do
+      expect(server).not_to be_unknown
+      operation
+      expect(server).not_to be_unknown
     end
+  end
 
-    shared_examples_for 'marks server unknown' do
-      it 'marks server unknown' do
-        expect(server).not_to be_unknown
-        operation
-        expect(server).to be_unknown
-      end
+  shared_examples_for 'requests server scan' do
+    it 'requests server scan' do
+      expect(server.monitor.scan_semaphore).to receive(:signal)
+      operation
     end
+  end
 
-    shared_examples_for 'does not mark server unknown' do
-      it 'does not mark server unknown' do
-        expect(server).not_to be_unknown
-        operation
-        expect(server).not_to be_unknown
-      end
+  shared_examples_for 'does not request server scan' do
+    it 'does not request server scan' do
+      expect(server.monitor.scan_semaphore).not_to receive(:signal)
+      operation
     end
+  end
 
-    shared_examples_for 'requests server scan' do
-      it 'requests server scan' do
-        expect(server.monitor.scan_semaphore).to receive(:signal)
-        operation
-      end
+  shared_examples_for 'clears connection pool' do
+    it 'clears connection pool' do
+      generation = server.pool.generation
+      operation
+      new_generation = server.pool.generation
+      # Temporary hack to allow repeated pool clears
+      expect(new_generation).to be >= generation + 1
     end
+  end
 
-    shared_examples_for 'does not request server scan' do
-      it 'does not request server scan' do
-        expect(server.monitor.scan_semaphore).not_to receive(:signal)
-        operation
-      end
+  shared_examples_for 'does not clear connection pool' do
+    it 'does not clear connection pool' do
+      generation = server.pool.generation
+      operation
+      new_generation = server.pool.generation
+      expect(new_generation).to eq(generation)
     end
+  end
+
+  describe 'when there is an error during an operation' do
 
-    shared_examples_for 'clears connection pool' do
-      it 'clears connection pool' do
-        generation = server.pool.generation
-        operation
-        new_generation = server.pool.generation
-        expect(new_generation).to eq(generation + 1)
+    before do
+      wait_for_all_servers(client.cluster)
+      # we also need a connection to the primary so that our error
+      # expectations do not get triggered during handshakes which
+      # have different behavior from non-handshake errors
+      client.database.command(ping: 1)
+      client.cluster.servers_list.each do |server|
+        server.monitor.stop!
       end
     end
 
-    shared_examples_for 'does not clear connection pool' do
-      it 'does not clear connection pool' do
-        generation = server.pool.generation
-        operation
-        new_generation = server.pool.generation
-        expect(new_generation).to eq(generation)
-      end
+    let(:operation) do
+      expect_any_instance_of(Mongo::Server::Connection).to receive(:deliver).and_return(reply)
+      expect do
+        client.database.command(ping: 1)
+      end.to raise_error(Mongo::Error::OperationFailure, exception_message)
     end
 
     shared_examples_for 'not master or node recovering' do
@@ -88,7 +89,8 @@
       context 'server 4.2 or higher' do
         min_server_fcv '4.2'
 
-        it_behaves_like 'does not clear connection pool'
+        # Due to RUBY-1894 backport, the pool is cleared here
+        it_behaves_like 'clears connection pool'
       end
 
       context 'server 4.0 or lower' do
@@ -170,4 +172,39 @@
       end
     end
   end
+
+  # These tests fail intermittently in Evergreen
+  describe 'when there is an error on monitoring connection', retry: 3 do
+    let(:client) do
+      authorized_client_without_any_retries.with(
+        connect_timeout: 1, socket_timeout: 1)
+    end
+
+    let(:operation) do
+      expect(server.monitor.connection).not_to be nil
+      expect(server.monitor.connection).to receive(:ismaster).at_least(:once).and_raise(exception)
+      server.monitor.scan_semaphore.broadcast
+      6.times do
+        sleep 0.5
+        if server.unknown?
+          break
+        end
+      end
+      expect(server).to be_unknown
+    end
+
+    context 'non-timeout network error' do
+      let(:exception) { Mongo::Error::SocketError }
+
+      it_behaves_like 'marks server unknown'
+      it_behaves_like 'clears connection pool'
+    end
+
+    context 'network timeout' do
+      let(:exception) { Mongo::Error::SocketTimeoutError }
+
+      it_behaves_like 'marks server unknown'
+      it_behaves_like 'clears connection pool'
+    end
+  end
 end

spec/integration/step_down_spec.rb

@@ -160,7 +160,8 @@
           collection.insert_one(test: 1)
         end.to raise_error(Mongo::Error::OperationFailure, /10107/)
 
-        expect(event_subscriber.select_published_events(Mongo::Monitoring::Event::Cmap::PoolCleared).count).to eq(0)
+        # Temporarily add 1 due to RUBY-1894 backport
+        expect(event_subscriber.select_published_events(Mongo::Monitoring::Event::Cmap::PoolCleared).count).to eq(0+1)
       end
     end
 
@@ -179,7 +180,8 @@
           collection.insert_one(test: 1)
         end.to raise_error(Mongo::Error::OperationFailure, /10107/)
 
-        expect(event_subscriber.select_published_events(Mongo::Monitoring::Event::Cmap::PoolCleared).count).to eq(1)
+        # Temporarily add 1 due to RUBY-1894 backport
+        expect(event_subscriber.select_published_events(Mongo::Monitoring::Event::Cmap::PoolCleared).count).to eq(1+1)
       end
     end
 
@@ -196,7 +198,8 @@
           collection.insert_one(test: 1)
         end.to raise_error(Mongo::Error::OperationFailure, /11600/)
 
-        expect(event_subscriber.select_published_events(Mongo::Monitoring::Event::Cmap::PoolCleared).count).to eq(1)
+        # Temporarily add 1 due to RUBY-1894 backport
+        expect(event_subscriber.select_published_events(Mongo::Monitoring::Event::Cmap::PoolCleared).count).to eq(1+1)
       end
     end
   end

spec/mongo/server/connection_spec.rb

@@ -270,6 +270,7 @@ class ConnectionSpecTestException < Exception; end
       end
     end
 
+=begin These assertions require a working cluster with working SDAM flow, which the tests do not configure
     shared_examples_for 'does not disconnect connection pool' do
       it 'does not disconnect non-monitoring sockets' do
         allow(server).to receive(:pool).and_return(pool)
@@ -285,6 +286,7 @@ class ConnectionSpecTestException < Exception; end
         error
       end
     end
+=end
 
     let(:auth_mechanism) do
       if ClusterConfig.instance.server_version >= '3'
@@ -331,14 +333,14 @@ class ConnectionSpecTestException < Exception; end
             expect(error).to be_a(Mongo::Auth::Unauthorized)
           end
 
-          it_behaves_like 'disconnects connection pool'
+          #it_behaves_like 'disconnects connection pool'
           it_behaves_like 'keeps server type and topology'
         end
 
         # need a separate context here, otherwise disconnect expectation
         # is ignored due to allowing disconnects in the other context
         context 'checking pool disconnection' do
-          it_behaves_like 'disconnects connection pool'
+          #it_behaves_like 'disconnects connection pool'
         end
       end
 
@@ -368,7 +370,7 @@ class ConnectionSpecTestException < Exception; end
           expect(error).to be_a(Mongo::Error::SocketTimeoutError)
         end
 
-        it_behaves_like 'does not disconnect connection pool'
+        #it_behaves_like 'does not disconnect connection pool'
         it_behaves_like 'keeps server type and topology'
       end
 
@@ -398,7 +400,7 @@ class ConnectionSpecTestException < Exception; end
           expect(error).to be_a(Mongo::Error::SocketError)
         end
 
-        it_behaves_like 'disconnects connection pool'
+        #it_behaves_like 'disconnects connection pool'
         it_behaves_like 'marks server unknown'
       end
 
@@ -706,7 +708,8 @@ class ConnectionSpecTestException < Exception; end
         end
 
         it 'disconnects connection pool' do
-          expect(server.pool).to receive(:disconnect!)
+          # Allow multiple calls due to RUBY-1894 backport
+          expect(server.pool).to receive(:disconnect!).at_least(:once)
           result
         end
 
@@ -739,10 +742,12 @@ class ConnectionSpecTestException < Exception; end
           expect(connection).to_not be_connected
         end
 
+=begin These assertions require a working cluster with working SDAM flow, which the tests do not configure
         it 'does not disconnect connection pool' do
           expect(server.pool).not_to receive(:disconnect!)
           result
         end
+=end
 
         it 'does not mark server unknown' do
           expect(server).not_to be_unknown

spec/mongo/server/monitor/connection_spec.rb

@@ -4,7 +4,7 @@
   clean_slate
 
   let(:address) do
-    Mongo::Address.new(ClusterConfig.instance.primary_address_str, options)
+    Mongo::Address.new(ClusterConfig.instance.primary_address, options)
   end
 
   declare_topology_double
@@ -24,6 +24,8 @@
                       Mongo::Event::Listeners.new, options)
   end
 
+  let(:monitor) { server.monitor }
+
   let(:connection) do
     # NB this connection is set up in the background thread,
     # when the :scan option to client is changed to default to false