RUBY-775 Fix recursive locking with both immutable and mutable pool manager state

estolfo · estolfo · commit da29ab7d9730 · 2014-05-30T12:20:45.000-04:00
diff --git a/lib/mongo/connection/pool_manager.rb b/lib/mongo/connection/pool_manager.rb
@@ -17,6 +17,11 @@ class PoolManager
     include ThreadLocalVariableManager
 
     attr_reader :client,
+                :hosts,
+                :pools,
+                :secondaries,
+                :secondary_pools,
+                :arbiters,
                 :primary,
                 :primary_pool,
                 :seeds,
@@ -36,12 +41,12 @@ def initialize(client, seeds=[])
       @client                                   = client
       @seeds                                    = seeds
 
+      initialize_immutable_state
+      initialize_mutable_state
+
       @pools                                    = Set.new
       @primary                                  = nil
       @primary_pool                             = nil
-      @secondaries                              = Set.new
-      @secondary_pools                          = []
-      @hosts                                    = Set.new
       @members                                  = Set.new
       @refresh_required                         = false
       @max_bson_size                            = DEFAULT_MAX_BSON_SIZE
@@ -70,6 +75,7 @@ def connect
           thread_local[:locks][:connecting_manager] = false
         end
       end
+      clone_state
     end
 
     def refresh!(additional_seeds)
@@ -140,47 +146,6 @@ def read
       read_pool.host_port
     end
 
-    def hosts
-      @connect_mutex.synchronize do
-        @hosts.nil? ? nil : @hosts.clone
-      end
-    end
-
-    def pools
-      @connect_mutex.synchronize do
-        @pools.nil? ? nil : @pools.clone
-      end
-    end
-
-    def secondaries
-      @connect_mutex.synchronize do
-        @secondaries.nil? ? nil : @secondaries.clone
-      end
-    end
-
-    def secondary_pools
-      @connect_mutex.synchronize do
-        @secondary_pools.nil? ? nil : @secondary_pools.clone
-      end
-    end
-
-    def arbiters
-      @connect_mutex.synchronize do
-        @arbiters.nil? ? nil : @arbiters.clone
-      end
-    end
-
-    def state_snapshot
-      @connect_mutex.synchronize do
-        { :pools           => @pools.nil?           ? nil : @pools.clone,
-          :secondaries     => @secondaries.nil?     ? nil : @secondaries.clone,
-          :secondary_pools => @secondary_pools.nil? ? nil : @secondary_pools.clone,
-          :hosts           => @hosts.nil?           ? nil : @hosts.clone,
-          :arbiters        => @arbiters.nil?        ? nil : @arbiters.clone
-        }
-      end
-    end
-
     private
 
     def update_max_sizes
@@ -203,7 +168,7 @@ def validate_existing_member(current_config, member)
 
     # For any existing members, close and remove any that are unhealthy or already closed.
     def disconnect_old_members
-      @pools.reject!   {|pool| !pool.healthy? }
+      @pools_mutable.reject!   {|pool| !pool.healthy? }
       @members.reject! {|node| !node.healthy? }
     end
 
@@ -243,13 +208,13 @@ def connect_to_members
     def initialize_pools(members)
       @primary_pool = nil
       @primary = nil
-      @secondaries.clear
-      @secondary_pools.clear
-      @hosts.clear
+      @secondaries_mutable.clear
+      @secondary_pools_mutable.clear
+      @hosts_mutable.clear
 
       members.each do |member|
         member.last_state = nil
-        @hosts << member.host_string
+        @hosts_mutable << member.host_string
         if member.primary?
           assign_primary(member)
         elsif member.secondary?
@@ -258,37 +223,37 @@ def initialize_pools(members)
         end
       end
 
-      @arbiters = members.first.arbiters
+      @arbiters_mutable = members.first.arbiters
     end
 
     def assign_primary(member)
       member.last_state = :primary
       @primary = member.host_port
-      if existing = @pools.detect {|pool| pool.node == member }
+      if existing = @pools_mutable.detect {|pool| pool.node == member }
         @primary_pool = existing
       else
         @primary_pool = Pool.new(self.client, member.host, member.port,
           :size => self.client.pool_size,
           :timeout => self.client.pool_timeout,
           :node => member
         )
-        @pools << @primary_pool
+        @pools_mutable << @primary_pool
       end
     end
 
     def assign_secondary(member)
       member.last_state = :secondary
-      @secondaries << member.host_port
-      if existing = @pools.detect {|pool| pool.node == member }
-        @secondary_pools << existing
+      @secondaries_mutable << member.host_port
+      if existing = @pools_mutable.detect {|pool| pool.node == member }
+        @secondary_pools_mutable << existing
       else
         pool = Pool.new(self.client, member.host, member.port,
           :size => self.client.pool_size,
           :timeout => self.client.pool_timeout,
           :node => member
         )
-        @secondary_pools << pool
-        @pools << pool
+        @secondary_pools_mutable << pool
+        @pools_mutable << pool
       end
     end
 
@@ -321,5 +286,35 @@ def copy_members
        end
        members
     end
+
+    def initialize_immutable_state
+      @hosts           = Set.new.freeze
+      @pools           = Set.new.freeze
+      @secondaries     = Set.new.freeze
+      @secondary_pools = [].freeze
+      @arbiters        = [].freeze
+    end
+
+    def initialize_mutable_state
+      @hosts_mutable           = Set.new
+      @pools_mutable           = Set.new
+      @secondaries_mutable     = Set.new
+      @secondary_pools_mutable = []
+      @arbiters_mutable        = []
+    end
+
+    def clone_state
+      @hosts           = @hosts_mutable.clone
+      @pools           = @pools_mutable.clone
+      @secondaries     = @secondaries_mutable.clone
+      @secondary_pools = @secondary_pools_mutable.clone
+      @arbiters        = @arbiters_mutable.clone
+
+      @hosts.freeze
+      @pools.freeze
+      @secondaries.freeze
+      @secondary_pools.freeze
+      @arbiters.freeze
+    end
   end
 end
diff --git a/test/replica_set/refresh_test.rb b/test/replica_set/refresh_test.rb
@@ -123,6 +123,24 @@ def test_concurrent_refreshes
     end
   end
 
+
+  def test_manager_recursive_locking
+    # See RUBY-775
+    # This tests that there isn't recursive locking when a pool manager reconnects
+    # to all replica set members. The bug in RUBY-775 occurred because the same lock
+    # acquired in order to connect the pool manager was used to read the pool manager's
+    # state.
+    client = MongoReplicaSetClient.new(@rs.repl_set_seeds)
+
+    cursor = client[TEST_DB]['rs-refresh-test'].find
+    client.stubs(:receive_message).raises(ConnectionFailure)
+    client.manager.stubs(:refresh_required?).returns(true)
+    client.manager.stubs(:check_connection_health).returns(true)
+    assert_raise ConnectionFailure do
+      cursor.next
+    end
+  end
+
 =begin
   def test_automated_refresh_with_removed_node
     client = MongoReplicaSetClient.new(@rs.repl_set_seeds,
diff --git a/test/unit/pool_manager_test.rb b/test/unit/pool_manager_test.rb
@@ -106,37 +106,6 @@ class PoolManagerUnitTest < Test::Unit::TestCase
       assert_equal [['localhost', 27020]], manager.arbiters
     end
 
-    should "return clones of pool lists" do
-
-      @db.stubs(:command).returns(
-        # First call to get a socket.
-        @ismaster.merge({'ismaster' => true}),
-
-        # Subsequent calls to configure pools.
-        @ismaster.merge({'ismaster' => true}),
-        @ismaster.merge({'secondary' => true, 'maxBsonObjectSize' => 500}),
-        @ismaster.merge({'secondary' => true, 'maxMessageSizeBytes' => 700}),
-        @ismaster.merge({'arbiterOnly' => true})
-      )
-
-      seeds = [['localhost', 27017], ['localhost', 27018]]
-      manager = Mongo::PoolManager.new(@client, seeds)
-      @client.stubs(:local_manager).returns(manager)
-      manager.connect
-
-      assert_not_equal manager.instance_variable_get(:@arbiters).object_id, manager.arbiters.object_id
-      assert_not_equal manager.instance_variable_get(:@secondaries).object_id, manager.secondaries.object_id
-      assert_not_equal manager.instance_variable_get(:@secondary_pools).object_id, manager.secondary_pools.object_id
-      assert_not_equal manager.instance_variable_get(:@hosts).object_id, manager.hosts.object_id
-      assert_not_equal manager.instance_variable_get(:@pools).object_id, manager.pools.object_id
-
-      assert_not_equal manager.instance_variable_get(:@arbiters).object_id, manager.state_snapshot[:arbiters].object_id
-      assert_not_equal manager.instance_variable_get(:@secondaries).object_id, manager.state_snapshot[:secondaries].object_id
-      assert_not_equal manager.instance_variable_get(:@secondary_pools).object_id, manager.state_snapshot[:secondary_pools].object_id
-      assert_not_equal manager.instance_variable_get(:@hosts).object_id, manager.state_snapshot[:hosts].object_id
-      assert_not_equal manager.instance_variable_get(:@pools).object_id, manager.state_snapshot[:pools].object_id
-    end
-
   end
 
 end
