Spread both max_lifetime and disconnect_all over given intervals

Author: josevalim

integration_test/connection_pool/disconnect_all_test.exs

@@ -59,12 +59,11 @@ defmodule TestPoolDisconnectAll do
 
     {:ok, agent} = A.start_link(stack)
 
-    opts = [agent: agent, parent: self(), idle_interval: 50]
+    opts = [agent: agent, parent: self(), idle_interval: 200]
     {:ok, pool} = P.start_link(opts)
     assert P.execute(pool, %Q{}, [:param]) == {:ok, %Q{}, %R{}}
 
-    # High intervals do not affect ping because those are always disconnected.
-    P.disconnect_all(pool, 45_000)
+    P.disconnect_all(pool, 0)
     assert_receive :disconnecting
     assert P.execute(pool, %Q{}, [:param]) == {:ok, %Q{}, %R{}}
     assert P.execute(pool, %Q{}, [:param]) == {:ok, %Q{}, %R{}}

lib/db_connection/connection_pool.ex

@@ -6,11 +6,6 @@ defmodule DBConnection.ConnectionPool do
 
   You're not supposed to call any functions on this pool directly, but only pass this
   as the value of the `:pool` option in functions such as `DBConnection.start_link/2`.
-
-  `disconnect_all/3`, which by default will result in connections being
-  reestablished, can be called periodically to recycle checked-in connections
-  after a maximum lifetime is reached. `Ecto SQL` users may find it at
-  https://hexdocs.pm/ecto_sql/Ecto.Adapters.SQL.html#disconnect_all/3
   """
 
   use GenServer
@@ -68,7 +63,7 @@ defmodule DBConnection.ConnectionPool do
           nil
       end
 
-    ts = {System.monotonic_time(), 0, max_lifetime}
+    ts = {nil, max_lifetime}
     {:ok, _} = DBConnection.ConnectionPool.Pool.start_supervised(queue, mod, opts)
     target = Keyword.get(opts, :queue_target, @queue_target)
     interval = Keyword.get(opts, :queue_interval, @queue_interval)
@@ -114,8 +109,8 @@ defmodule DBConnection.ConnectionPool do
   end
 
   def handle_call({:disconnect_all, interval}, _from, {type, queue, codel, ts}) do
-    {_, _, max_lifetime} = ts
-    ts = {System.monotonic_time(), interval, max_lifetime}
+    {_, max_lifetime} = ts
+    ts = {{System.monotonic_time(), interval}, max_lifetime}
     {:reply, :ok, {type, queue, codel, ts}}
   end
 
@@ -165,9 +160,9 @@ defmodule DBConnection.ConnectionPool do
 
         case :ets.info(holder, :owner) do
           ^owner ->
-            {time, interval, max_lifetime} = ts
+            {interval, max_lifetime} = ts
 
-            if Holder.maybe_disconnect(holder, time, interval, max_lifetime) do
+            if Holder.maybe_disconnect(holder, interval, max_lifetime) do
               {:noreply, data}
             else
               handle_checkin(holder, extra, data)
@@ -239,8 +234,8 @@ defmodule DBConnection.ConnectionPool do
          {queued_in_native, holder} = key when queued_in_native <= past_in_native <-
            :ets.first(queue) do
       :ets.delete(queue, key)
-      {time, _interval, max_lifetime} = ts
-      Holder.maybe_disconnect(holder, time, 0, max_lifetime) or Holder.handle_ping(holder)
+      {interval, max_lifetime} = ts
+      Holder.maybe_disconnect(holder, interval, max_lifetime) or Holder.handle_ping(holder)
       drop_idle(past_in_native, limit - 1, status, queue, codel, ts)
     else
       _ ->

lib/db_connection/holder.ex

@@ -252,13 +252,13 @@ defmodule DBConnection.Holder do
     handle_done(holder, &DBConnection.Connection.stop/3, err)
   end
 
-  @spec maybe_disconnect(t, integer, non_neg_integer, {integer, non_neg_integer} | nil) ::
+  @spec maybe_disconnect(t, {integer, non_neg_integer} | nil, {integer, non_neg_integer} | nil) ::
           boolean()
-  def maybe_disconnect(holder, start, interval_ms, lifetime) do
-    ts = :ets.lookup_element(holder, :conn, conn(:connected_at) + 1)
+  def maybe_disconnect(_holder, nil, nil), do: false
 
-    disconnect_all_reason(start, interval_ms, ts, holder) ||
-      max_lifetime_reason(lifetime, ts, holder)
+  def maybe_disconnect(holder, interval, lifetime) do
+    ts = :ets.lookup_element(holder, :conn, conn(:connected_at) + 1)
+    disconnect_all_reason(holder, ts, interval) || max_lifetime_reason(holder, ts, lifetime)
   rescue
     _ -> false
   else
@@ -270,26 +270,22 @@ defmodule DBConnection.Holder do
       handle_disconnect(holder, DBConnection.ConnectionError.exception(opts))
   end
 
-  defp max_lifetime_reason(nil, _ts, _holder), do: nil
+  defp max_lifetime_reason(_holder, _ts, nil), do: nil
 
-  defp max_lifetime_reason({start, interval_ms}, ts, holder) do
+  defp max_lifetime_reason(holder, ts, {min_lifetime, interval_ms}) do
     elapsed = System.monotonic_time() - ts
 
     # First check if passed start then check if also the interval
-    if elapsed > start and elapsed > hash_holder(holder, interval_ms) + start do
+    if elapsed > min_lifetime and elapsed > hash_holder(holder, interval_ms) + min_lifetime do
       "max_lifetime exceeded"
     end
   end
 
-  defp disconnect_all_reason(start, interval_ms, ts, holder) do
-    disconnect? =
-      cond do
-        ts >= start -> false
-        interval_ms == 0 -> true
-        true -> System.monotonic_time() > hash_holder(holder, interval_ms) + start
-      end
+  defp disconnect_all_reason(_holder, _ts, nil), do: nil
 
-    if disconnect? do
+  defp disconnect_all_reason(holder, ts, {disconnect_start, interval_ms}) do
+    if disconnect_start > ts and
+         System.monotonic_time() > hash_holder(holder, interval_ms) + disconnect_start do
       "disconnect_all requested"
     end
   end