Track checked out created connections across reloads (#192)

* Add test cases for reload that assert connections are not created if at the connection pool size

The TimedStack class currently allows

* Ensure TimedStack does not create more connections than max size even after reload.

* remove decrement_created method in favor of inline

* add test case to ensure shutdown and reap can be called after errors

lib/connection_pool/timed_stack.rb

@@ -41,6 +41,7 @@ def initialize(size = 0, &block)
   def push(obj, options = {})
     @mutex.synchronize do
       if @shutdown_block
+        @created -= 1 unless @created == 0
         @shutdown_block.call(obj)
       else
         store_connection obj, options
@@ -170,9 +171,9 @@ def fetch_connection(options = nil)
   def shutdown_connections(options = nil)
     while connection_stored?(options)
       conn = fetch_connection(options)
+      @created -= 1 unless @created == 0
       @shutdown_block.call(conn)
     end
-    @created = 0
   end
 
   ##
@@ -184,8 +185,7 @@ def shutdown_connections(options = nil)
   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
+    @created -= 1 unless @created == 0
 
     @que.shift.first
   end

test/test_connection_pool.rb

@@ -407,6 +407,17 @@ def test_shutdown_is_executed_for_all_connections
     assert_equal [["shutdown"]] * 3, recorders.map { |r| r.calls }
   end
 
+  def test_checkout_after_reload_cannot_create_new_connections_beyond_size
+    pool = ConnectionPool.new(size: 1) { Object.new }
+    threads = use_pool pool, 1
+    pool.reload {}
+    assert_raises ConnectionPool::TimeoutError do
+      pool.checkout(timeout: 0)
+    end
+  ensure
+    kill_threads(threads) if threads
+  end
+
   def test_raises_error_after_shutting_down
     pool = ConnectionPool.new(size: 1) { true }
 

test/test_connection_pool_timed_stack.rb

@@ -33,6 +33,27 @@ def test_length
     assert_equal 1, stack.length
   end
 
+  def test_length_after_shutdown_reload_for_no_create_stack
+    assert_equal 0, @stack.length
+
+    @stack.push(Object.new)
+
+    assert_equal 1, @stack.length
+
+    @stack.shutdown(reload: true) {}
+
+    assert_equal 0, @stack.length
+  end
+
+  def test_length_after_shutdown_reload_with_checked_out_conn
+    stack = ConnectionPool::TimedStack.new(1) { Object.new }
+    conn = stack.pop
+    stack.shutdown(reload: true) {}
+    assert_equal 0, stack.length
+    stack.push(conn)
+    assert_equal 1, stack.length
+  end
+
   def test_idle
     stack = ConnectionPool::TimedStack.new(1) { Object.new }
 
@@ -94,6 +115,22 @@ def test_pop_full
     assert_empty stack
   end
 
+  def test_pop_full_with_extra_conn_pushed
+    stack = ConnectionPool::TimedStack.new(1) { Object.new }
+    popped = stack.pop
+
+    stack.push(Object.new)
+    stack.push(popped)
+
+    assert_equal 2, stack.length
+
+    stack.shutdown(reload: true) {}
+
+    assert_equal 1, stack.length
+    stack.pop
+    assert_raises(ConnectionPool::TimeoutError) { stack.pop(0) }
+  end
+
   def test_pop_wait
     thread = Thread.start {
       @stack.pop
@@ -126,6 +163,15 @@ def test_pop_shutdown_reload
     refute_equal object, stack.pop
   end
 
+  def test_pop_raises_error_if_shutdown_reload_is_run_and_connection_is_still_in_use
+    stack = ConnectionPool::TimedStack.new(1) { Object.new }
+    stack.pop
+    stack.shutdown(reload: true) {}
+    assert_raises ConnectionPool::TimeoutError do
+      stack.pop(0)
+    end
+  end
+
   def test_push
     stack = ConnectionPool::TimedStack.new(1) { Object.new }
 
@@ -162,6 +208,54 @@ def test_shutdown
     assert_empty @stack
   end
 
+  def test_shutdown_can_be_called_after_error
+    3.times { @stack.push Object.new }
+
+    called = []
+    closing_error = "error in closing connection"
+    raise_error = true
+    shutdown_proc = ->(conn) do
+      called << conn
+      if raise_error
+        raise_error = false
+        raise closing_error
+      end
+    end
+
+    assert_raises(closing_error) do
+      @stack.shutdown(&shutdown_proc)
+    end
+
+    assert_equal 1, called.size
+
+    @stack.shutdown(&shutdown_proc)
+    assert_equal 3, called.size
+  end
+
+  def test_reap_can_be_called_after_error
+    3.times { @stack.push Object.new }
+
+    called = []
+    closing_error = "error in closing connection"
+    raise_error = true
+    reap_proc = ->(conn) do
+      called << conn
+      if raise_error
+        raise_error = false
+        raise closing_error
+      end
+    end
+
+    assert_raises(closing_error) do
+      @stack.reap(0, &reap_proc)
+    end
+
+    assert_equal 1, called.size
+
+    @stack.reap(0, &reap_proc)
+    assert_equal 3, called.size
+  end
+
   def test_reap
     @stack.push Object.new