Skip to content

Commit

Permalink
Retry new connection on checkout after reap
Browse files Browse the repository at this point in the history
When we reap connections, we check if they are inactive (connected and
responding to ping in most adapters) and if so we remove the connection
instead of checking it back in.

However, in acquire_connection, we weren't checking after reaping
whether we were allowed to build a new connection, only whether an
existing one was in the available pool.

This still leaves a race condition where if the background reaper thread
runs while a thread is polling and finds a free inactive connection, it
could have the same issue and not wake the waiting thread. However we
don't really expect the reaper to solve this (it only runs every 60
seconds by default, far too slow to solve for a blocked thread). I think
this should be fixed, just separately.
  • Loading branch information
jhawthorn committed Nov 27, 2023
1 parent a4d5f1d commit afefa50
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 1 deletion.
Expand Up @@ -668,7 +668,13 @@ def acquire_connection(checkout_timeout)
conn
else
reap
@available.poll(checkout_timeout)
# Retry after reaping, which may return an available connection,
# remove an inactive connection, or both
if conn = @available.poll || try_to_checkout_new_connection
conn
else
@available.poll(checkout_timeout)
end
end
end

Expand Down
24 changes: 24 additions & 0 deletions activerecord/test/cases/connection_pool_test.rb
Expand Up @@ -215,6 +215,30 @@ def test_reap_inactive
@pool.connections.each { |conn| conn.close if conn.in_use? }
end

def test_inactive_are_returned_from_dead_thread
ready = Concurrent::CountDownLatch.new
@pool.instance_variable_set(:@size, 1)

child = new_thread do
@pool.checkout
ready.count_down
stop_thread
end

pass_to(child) until ready.wait(0)

assert_equal 1, active_connections(@pool).size

child.terminate
child.join

@pool.checkout

assert_equal 1, active_connections(@pool).size
ensure
@pool.connections.each { |conn| conn.close if conn.in_use? }
end

def test_idle_timeout_configuration
@pool.disconnect!

Expand Down

0 comments on commit afefa50

Please sign in to comment.