mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
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 of9e457a8654
and essentially all of4367d2f05c
This commit is contained in:
parent
d9d865aa40
commit
5e024070ab
2 changed files with 36 additions and 4 deletions
|
@ -363,7 +363,7 @@ module ActiveRecord
|
||||||
conn.expire
|
conn.expire
|
||||||
end
|
end
|
||||||
|
|
||||||
release owner
|
release conn, owner
|
||||||
|
|
||||||
@available.add conn
|
@available.add conn
|
||||||
end
|
end
|
||||||
|
@ -376,7 +376,7 @@ module ActiveRecord
|
||||||
@connections.delete conn
|
@connections.delete conn
|
||||||
@available.delete conn
|
@available.delete conn
|
||||||
|
|
||||||
release conn.owner
|
release conn, conn.owner
|
||||||
|
|
||||||
@available.add checkout_new_connection if @available.any_waiting?
|
@available.add checkout_new_connection if @available.any_waiting?
|
||||||
end
|
end
|
||||||
|
@ -424,10 +424,12 @@ module ActiveRecord
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def release(owner)
|
def release(conn, owner)
|
||||||
thread_id = owner.object_id
|
thread_id = owner.object_id
|
||||||
|
|
||||||
@reserved_connections.delete thread_id
|
if @reserved_connections[thread_id] == conn
|
||||||
|
@reserved_connections.delete thread_id
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def new_connection
|
def new_connection
|
||||||
|
|
|
@ -35,6 +35,22 @@ class PooledConnectionsTest < ActiveRecord::TestCase
|
||||||
end
|
end
|
||||||
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
|
def test_pooled_connection_checkin_one
|
||||||
checkout_checkin_connections 1, 2
|
checkout_checkin_connections 1, 2
|
||||||
assert_equal 2, @connection_count
|
assert_equal 2, @connection_count
|
||||||
|
@ -42,6 +58,20 @@ class PooledConnectionsTest < ActiveRecord::TestCase
|
||||||
assert_equal 1, ActiveRecord::Base.connection_pool.connections.size
|
assert_equal 1, ActiveRecord::Base.connection_pool.connections.size
|
||||||
end
|
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
|
private
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue