Skip to content

Commit

Permalink
Ability to optionally drop all connections after fork (#177)
Browse files Browse the repository at this point in the history
There was a recent feature to automatically drop all connections after fork. This is quite nice
and makes sense.

However, for some rails app that usually follow the fork model (like w/ unicorn/puma), and additionally
have some logic to fork processes to perform internal business logic that doesn't rely or use ConnectionPool,
the application can observe Redis connection issues or resets. These forks can happen during application run time.
Like ours.

In such a case, it'd be nice to not automatically drop all the connections, since the underlying process isn't working
with Redis/ConnectionPool, and as a sideeffect the pool in the primary process is impacted.

This PR proposes a new attribute auto_reload_after_fork as a config option. By default it is true. However, application
users can turn it to false and not opt in for the feature to auto drop connections after fork.

This could be quite useful for us
  • Loading branch information
shayonj committed May 19, 2023
1 parent 3d284f8 commit f7463bb
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 2 deletions.
9 changes: 7 additions & 2 deletions lib/connection_pool.rb
Expand Up @@ -36,9 +36,10 @@ class TimeoutError < ::Timeout::Error; end
# Accepts the following options:
# - :size - number of connections to pool, defaults to 5
# - :timeout - amount of time to wait for a connection if none currently available, defaults to 5 seconds
# - :auto_reload_after_fork - automatically drop all connections after fork, defaults to true
#
class ConnectionPool
DEFAULTS = {size: 5, timeout: 5}
DEFAULTS = {size: 5, timeout: 5, auto_reload_after_fork: true}

def self.wrap(options, &block)
Wrapper.new(options, &block)
Expand All @@ -50,11 +51,12 @@ def self.wrap(options, &block)

def self.after_fork
INSTANCES.values.each do |pool|
next unless pool.auto_reload_after_fork

# We're on after fork, so we know all other threads are dead.
# All we need to do is to ensure the main thread doesn't have a
# checked out connection
pool.checkin(force: true)

pool.reload do |connection|
# Unfortunately we don't know what method to call to close the connection,
# so we try the most common one.
Expand Down Expand Up @@ -92,6 +94,7 @@ def initialize(options = {}, &block)

@size = Integer(options.fetch(:size))
@timeout = options.fetch(:timeout)
@auto_reload_after_fork = options.fetch(:auto_reload_after_fork)

@available = TimedStack.new(@size, &block)
@key = :"pool-#{@available.object_id}"
Expand Down Expand Up @@ -159,6 +162,8 @@ def reload(&block)

# Size of this connection pool
attr_reader :size
# Automatically drop all connections after fork
attr_reader :auto_reload_after_fork

# Number of pool entries available for checkout at this instant.
def available
Expand Down
12 changes: 12 additions & 0 deletions test/test_connection_pool.rb
Expand Up @@ -559,6 +559,18 @@ def test_after_fork_callback
refute_equal(prefork_connection, pool.with { |c| c })
end

def test_after_fork_callback_being_skipped
skip("MRI feature") unless Process.respond_to?(:fork)
GC.start # cleanup instances created by other tests

pool = ConnectionPool.new(size: 2, auto_reload_after_fork: false) { NetworkConnection.new }
prefork_connection = pool.with { |c| c }
assert_equal(prefork_connection, pool.with { |c| c })
ConnectionPool.after_fork
assert_equal(prefork_connection, pool.with { |c| c })
end


def test_after_fork_callback_checkin
skip("MRI feature") unless Process.respond_to?(:fork)
GC.start # cleanup instances created by other tests
Expand Down

0 comments on commit f7463bb

Please sign in to comment.