From 3360742396a00c9e2ff9838373788bed432d5ea7 Mon Sep 17 00:00:00 2001 From: Devin Christensen Date: Wed, 12 Apr 2017 14:45:10 -0600 Subject: Switch to LIFO for the connection pool Using a FIFO for the connection pool can lead to issues when there are upstream components (pgbouncer, haproxy, etc.) that terminate connections that are idle after a period of time. Switching to a LIFO reduces the probability that a thread will checkout a connection that is about to be closed by an idle timeout in an upstream component. --- .../lib/active_record/connection_adapters/abstract/connection_pool.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') 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 53dbbd8c21..ea76ff983e 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -80,7 +80,7 @@ module ActiveRecord # * private methods that require being called in a +synchronize+ blocks # are now explicitly documented class ConnectionPool - # Threadsafe, fair, FIFO queue. Meant to be used by ConnectionPool + # Threadsafe, fair, LIFO queue. Meant to be used by ConnectionPool # with which it shares a Monitor. But could be a generic Queue. # # The Queue in stdlib's 'thread' could replace this class except @@ -111,7 +111,7 @@ module ActiveRecord # Add +element+ to the queue. Never blocks. def add(element) synchronize do - @queue.push element + @queue.unshift element @cond.signal end end -- cgit v1.2.3 From 6116d7bc052839646f448b8403a7287f52b97ed7 Mon Sep 17 00:00:00 2001 From: Devin Christensen Date: Thu, 13 Apr 2017 13:56:42 -0600 Subject: Improve documentation and add test --- .../connection_adapters/abstract/connection_pool.rb | 11 ++++------- activerecord/test/cases/connection_pool_test.rb | 8 ++++++++ 2 files changed, 12 insertions(+), 7 deletions(-) (limited to 'activerecord') 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 ea76ff983e..3dc265622d 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -80,11 +80,8 @@ module ActiveRecord # * private methods that require being called in a +synchronize+ blocks # are now explicitly documented class ConnectionPool - # Threadsafe, fair, LIFO queue. Meant to be used by ConnectionPool - # with which it shares a Monitor. But could be a generic Queue. - # - # The Queue in stdlib's 'thread' could replace this class except - # stdlib's doesn't support waiting with a timeout. + # Threadsafe, fair, FIFO queue. Meant to be used by ConnectionPool + # with which it shares a Monitor. class Queue def initialize(lock = Monitor.new) @lock = lock @@ -111,7 +108,7 @@ module ActiveRecord # Add +element+ to the queue. Never blocks. def add(element) synchronize do - @queue.unshift element + @queue.push element @cond.signal end end @@ -173,7 +170,7 @@ module ActiveRecord # Removes and returns the head of the queue if possible, or +nil+. def remove - @queue.shift + @queue.pop end # Remove and return the head the queue if the number of diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb index 7e88c9cf7a..46081cc13d 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -203,6 +203,14 @@ module ActiveRecord end.join end + def test_checkout_order_is_fifo + conn1 = @pool.checkout + conn2 = @pool.checkout + @pool.checkin conn1 + @pool.checkin conn2 + assert_equal [conn2, conn1], 2.times.map { @pool.checkout } + end + # The connection pool is "fair" if threads waiting for # connections receive them in the order in which they began # waiting. This ensures that we don't timeout one HTTP request -- cgit v1.2.3 From 47d678d9b2ab6d3e888691e8327e657ff0b5dcb0 Mon Sep 17 00:00:00 2001 From: Devin Christensen Date: Thu, 13 Apr 2017 14:05:39 -0600 Subject: Fix typos --- .../lib/active_record/connection_adapters/abstract/connection_pool.rb | 2 +- activerecord/test/cases/connection_pool_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') 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 3dc265622d..9f660a419d 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -80,7 +80,7 @@ module ActiveRecord # * private methods that require being called in a +synchronize+ blocks # are now explicitly documented class ConnectionPool - # Threadsafe, fair, FIFO queue. Meant to be used by ConnectionPool + # Threadsafe, fair, LIFO queue. Meant to be used by ConnectionPool # with which it shares a Monitor. class Queue def initialize(lock = Monitor.new) diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb index 46081cc13d..790eed6007 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -203,7 +203,7 @@ module ActiveRecord end.join end - def test_checkout_order_is_fifo + def test_checkout_order_is_lifo conn1 = @pool.checkout conn2 = @pool.checkout @pool.checkin conn1 -- cgit v1.2.3