Avoid extra mutex contention

lib/connection_pool/timed_stack.rb

@@ -104,10 +104,16 @@ def reap(idle_seconds, &block)
     raise ArgumentError, "reap must receive a block" unless block
     raise ArgumentError, "idle_seconds must be a number" unless idle_seconds.is_a?(Numeric)
 
-    @mutex.synchronize do
-      reap_start_time = current_time
+    loop do
+      conn =
+        @mutex.synchronize do
+          raise ConnectionPool::PoolShuttingDownError if @shutdown_block
+
+          reserve_idle_connection(idle_seconds)
+        end
+      break unless conn
 
-      reap_idle_connections(idle_seconds, reap_start_time, &block)
+      block.call(conn)
     end
   end
 
@@ -171,26 +177,25 @@ def shutdown_connections(options = nil)
   ##
   # This is an extension point for TimedStack and is called with a mutex.
   #
-  # This method iterates over the connections in the stack and reaps the oldest idle connections one at a time until
-  # the first connection is not idle. This requires that the stack is kept in order of checked in time (oldest first).
+  # This method returns the oldest idle connection if it has been idle for more than idle_seconds.
+  # This requires that the stack is kept in order of checked in time (oldest first).
 
-  def reap_idle_connections(idle_seconds, reap_start_time, &reap_block)
-    while idle_connections?(idle_seconds, reap_start_time)
-      conn, _last_checked_out = @que.shift
-      reap_block.call(conn)
-    end
+  def reserve_idle_connection(idle_seconds)
+    return unless idle_connections?(idle_seconds)
+
+    # Decrement created unless this is a no create stack
+    @created -= 1 unless @max == 0
+
+    @que.shift.first
   end
 
   ##
   # This is an extension point for TimedStack and is called with a mutex.
   #
   # Returns true if the first connection in the stack has been idle for more than idle_seconds
 
-  def idle_connections?(idle_seconds, reap_start_time)
-    if connection_stored?
-      _conn, last_checked_out = @que.first
-      reap_start_time - last_checked_out > idle_seconds
-    end
+  def idle_connections?(idle_seconds)
+    connection_stored? && (current_time - @que.first.last > idle_seconds)
   end
 
   ##

test/test_connection_pool.rb

@@ -535,6 +535,16 @@ def test_idle_with_multiple_connections
     assert_equal 3, pool.idle
   end
 
+  def test_reap_raises_error_after_shutting_down
+    pool = ConnectionPool.new(size: 1) { true }
+
+    pool.shutdown {}
+
+    assert_raises ConnectionPool::PoolShuttingDownError do
+      pool.reap(0) {}
+    end
+  end
+
   def test_wrapper_wrapped_pool
     wrapper = ConnectionPool::Wrapper.new { NetworkConnection.new }
     assert_equal ConnectionPool, wrapper.wrapped_pool.class

test/test_connection_pool_timed_stack.rb

@@ -175,6 +175,18 @@ def test_reap
     assert_empty @stack
   end
 
+  def test_reap_full_stack
+    stack = ConnectionPool::TimedStack.new(1) { Object.new }
+    stack.push stack.pop
+
+    stack.reap(0) do |object|
+      nil
+    end
+
+    # Can still pop from the stack after reaping all connections
+    refute_nil stack.pop
+  end
+
   def test_reap_large_idle_seconds
     @stack.push Object.new
 
@@ -250,7 +262,7 @@ def test_reap_with_multiple_connections_and_zero_idle_seconds
     end
 
     assert_equal [conn1, conn2], called
-    assert_empty stack
+    assert_equal 0, stack.idle
   end
 
   def test_reap_with_multiple_connections_and_idle_seconds_outside_range