From 87fabdc2f3f618ef9954abe98e268025db6eb546 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Thu, 14 Jun 2018 18:22:02 +1000
Subject: [PATCH] FIX: correct pool reaper

This removes a freedom patch and replaces with a custom reaper thread
it also captures an issue where reaper would fail when connections where
empty
---
 config/discourse_defaults.conf                |  3 +-
 lib/discourse.rb                              | 17 +++++--
 lib/freedom_patches/pool_drainer.rb           | 47 -----------------
 .../freedom_patches/pool_drainer_spec.rb      | 51 -------------------
 4 files changed, 13 insertions(+), 105 deletions(-)
 delete mode 100644 lib/freedom_patches/pool_drainer.rb
 delete mode 100644 spec/components/freedom_patches/pool_drainer_spec.rb

diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf
index 7715cb29273..7af43d02fda 100644
--- a/config/discourse_defaults.conf
+++ b/config/discourse_defaults.conf
@@ -143,10 +143,9 @@ new_version_emails = true
 # will not work properly with huge numbers of open connections
 # reap connections from pool that are older than 30 seconds
 connection_reaper_age = 30
+
 # run reap check every 30 seconds
 connection_reaper_interval = 30
-# also reap any connections older than this
-connection_reaper_max_age = 600
 
 # set to relative URL (for subdirectory hosting)
 # IMPORTANT: path must not include a trailing /
diff --git a/lib/discourse.rb b/lib/discourse.rb
index f275f93c322..fe17d5362ba 100644
--- a/lib/discourse.rb
+++ b/lib/discourse.rb
@@ -435,7 +435,7 @@ module Discourse
   # after fork, otherwise Discourse will be
   # in a bad state
   def self.after_fork
-    # note: all this reconnecting may no longer be needed per https://github.com/redis/redis-rb/pull/414
+    # note: some of this reconnecting may no longer be needed per https://github.com/redis/redis-rb/pull/414
     MessageBus.after_fork
     SiteSetting.after_fork
     $redis._client.reconnect
@@ -487,7 +487,7 @@ module Discourse
       while true
         begin
           sleep GlobalSetting.connection_reaper_interval
-          reap_connections(GlobalSetting.connection_reaper_age, GlobalSetting.connection_reaper_max_age)
+          reap_connections(GlobalSetting.connection_reaper_age)
         rescue => e
           Discourse.warn_exception(e, message: "Error reaping connections")
         end
@@ -495,15 +495,22 @@ module Discourse
     end
   end
 
-  def self.reap_connections(idle, max_age)
+  def self.reap_connections(idle)
     pools = []
     ObjectSpace.each_object(ActiveRecord::ConnectionAdapters::ConnectionPool) { |pool| pools << pool }
 
+    i = 0
     pools.each do |pool|
+      i += 1
       # reap recovers connections that were aborted
       # eg a thread died or a dev forgot to check it in
-      pool.reap
-      pool.drain(idle.seconds, max_age.seconds)
+      # flush removes idle connections
+      # after fork we have "deadpools" so ignore them, they have been discarded
+      # so @connections is set to nil
+      if pool.connections
+        pool.reap
+        pool.flush(idle.seconds)
+      end
     end
   end
 
diff --git a/lib/freedom_patches/pool_drainer.rb b/lib/freedom_patches/pool_drainer.rb
deleted file mode 100644
index 3f99a1c61df..00000000000
--- a/lib/freedom_patches/pool_drainer.rb
+++ /dev/null
@@ -1,47 +0,0 @@
-class ActiveRecord::ConnectionAdapters::AbstractAdapter
-  module LastUseExtension
-    attr_reader :last_use, :first_use
-
-    def initialize(connection, logger = nil, pool = nil)
-      super
-      @last_use = false
-      @first_use = Time.now
-    end
-
-    def lease
-      @lock.synchronize do
-        unless in_use?
-          @last_use = Time.now
-          super
-        end
-      end
-    end
-  end
-
-  prepend LastUseExtension
-end
-
-class ActiveRecord::ConnectionAdapters::ConnectionPool
-  # drain all idle connections
-  # if idle_time is specified only connections idle for N seconds will be drained
-  def drain(idle_time = nil, max_age = nil)
-    return if !(@connections && @available)
-
-    idle_connections = synchronize do
-      @connections.select do |conn|
-        !conn.in_use? && ((idle_time && conn.last_use <= idle_time.seconds.ago) || (max_age && conn.first_use < max_age.seconds.ago))
-      end.each do |conn|
-        conn.lease
-
-        @available.delete conn
-        @connections.delete conn
-      end
-    end
-
-    idle_connections.each do |conn|
-      conn.disconnect!
-    end
-
-  end
-
-end
diff --git a/spec/components/freedom_patches/pool_drainer_spec.rb b/spec/components/freedom_patches/pool_drainer_spec.rb
deleted file mode 100644
index f538eeed4c6..00000000000
--- a/spec/components/freedom_patches/pool_drainer_spec.rb
+++ /dev/null
@@ -1,51 +0,0 @@
-require 'rails_helper'
-
-describe 'pool drainer' do
-  let(:pool) do
-    ActiveRecord::Base.connection_pool
-  end
-
-  it 'can correctly drain the connection pool' do
-
-    pool.reap
-    pool.drain(0)
-
-    old = pool.connections.length
-    expect(old).to eq(1)
-
-    Thread.new do
-      conn = pool.checkout
-      pool.checkin conn
-    end.join
-
-    expect(pool.connections.length).to eq(old + 1)
-
-    freeze_time 1.second.from_now
-    pool.drain(0)
-    expect(pool.connections.length).to eq(old)
-  end
-
-  it 'can drain with idle time setting' do
-    pool.drain
-    old = pool.connections.length
-    expect(old).to eq(1)
-
-    Thread.new do
-      conn = pool.checkout
-      pool.checkin conn
-    end.join
-
-    expect(pool.connections.length).to eq(old + 1)
-    pool.drain(1.minute)
-    expect(pool.connections.length).to eq(old + 1)
-
-    # make sure we don't corrupt internal state
-    20.times do
-      conn = pool.checkout
-      pool.checkin conn
-      pool.drain
-    end
-
-  end
-
-end