From 5e024070ab3766fada222053d1e0f1116aeb50a6 Mon Sep 17 00:00:00 2001
From: Matt Jones <al2o3cr@gmail.com>
Date: Tue, 23 Dec 2014 09:54:52 -0500
Subject: [PATCH] Fix connection leak when a thread checks in additional
 connections.

The code in `ConnectionPool#release` assumed that a single thread only
ever holds a single connection, and thus that releasing a connection
only requires the owning thread_id.

There is a trivial counterexample to this assumption: code that checks
out additional connections from the pool in the same thread. For
instance:

    connection_1 = ActiveRecord::Base.connection
    connection_2 = ActiveRecord::Base.connection_pool.checkout
    ActiveRecord::Base.connection_pool.checkin(connection_2)
    connection_3 = ActiveRecord::Base.connection

At this point, connection_1 has been removed from the
`@reserved_connections` hash, causing a NEW connection to be returned as
connection_3 and the loss of any tracking info on connection_1. As long
as the thread in this example lives, connection_1 will be inaccessible
and un-reapable. If this block of code runs more times than the size of
the connection pool in a single thread, every subsequent connection
attempt will timeout, as all of the available connections have been
leaked.

Reverts parts of 9e457a8654fa89fe329719f88ae3679aefb21e56 and
essentially all of 4367d2f05cbeda855820e25a08353d4b7b3457ac
---
 .../abstract/connection_pool.rb               | 10 ++++---
 .../test/cases/pooled_connections_test.rb     | 30 +++++++++++++++++++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
index 0fa00d03a3..3968b90341 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -363,7 +363,7 @@ module ActiveRecord
             conn.expire
           end
 
-          release owner
+          release conn, owner
 
           @available.add conn
         end
@@ -376,7 +376,7 @@ module ActiveRecord
           @connections.delete conn
           @available.delete conn
 
-          release conn.owner
+          release conn, conn.owner
 
           @available.add checkout_new_connection if @available.any_waiting?
         end
@@ -424,10 +424,12 @@ module ActiveRecord
         end
       end
 
-      def release(owner)
+      def release(conn, owner)
         thread_id = owner.object_id
 
-        @reserved_connections.delete thread_id
+        if @reserved_connections[thread_id] == conn
+          @reserved_connections.delete thread_id
+        end
       end
 
       def new_connection
diff --git a/activerecord/test/cases/pooled_connections_test.rb b/activerecord/test/cases/pooled_connections_test.rb
index 98888150a8..287a3f33ea 100644
--- a/activerecord/test/cases/pooled_connections_test.rb
+++ b/activerecord/test/cases/pooled_connections_test.rb
@@ -35,6 +35,22 @@ class PooledConnectionsTest < ActiveRecord::TestCase
     end
   end
 
+  def checkout_checkin_connections_loop(pool_size, loops)
+    ActiveRecord::Base.establish_connection(@connection.merge({:pool => pool_size, :checkout_timeout => 0.5}))
+    @connection_count = 0
+    @timed_out = 0
+    loops.times do
+      begin
+        conn = ActiveRecord::Base.connection_pool.checkout
+        ActiveRecord::Base.connection_pool.checkin conn
+        @connection_count += 1
+        ActiveRecord::Base.connection.tables
+      rescue ActiveRecord::ConnectionTimeoutError
+        @timed_out += 1
+      end
+    end
+  end
+
   def test_pooled_connection_checkin_one
     checkout_checkin_connections 1, 2
     assert_equal 2, @connection_count
@@ -42,6 +58,20 @@ class PooledConnectionsTest < ActiveRecord::TestCase
     assert_equal 1, ActiveRecord::Base.connection_pool.connections.size
   end
 
+  def test_pooled_connection_checkin_two
+    checkout_checkin_connections_loop 2, 3
+    assert_equal 3, @connection_count
+    assert_equal 0, @timed_out
+    assert_equal 2, ActiveRecord::Base.connection_pool.connections.size
+  end
+
+  def test_pooled_connection_remove
+    ActiveRecord::Base.establish_connection(@connection.merge({:pool => 2, :checkout_timeout => 0.5}))
+    old_connection = ActiveRecord::Base.connection
+    extra_connection = ActiveRecord::Base.connection_pool.checkout
+    ActiveRecord::Base.connection_pool.remove(extra_connection)
+    assert_equal ActiveRecord::Base.connection, old_connection
+  end
 
   private