Use connection establishment time for max_lifetime calculations

The holder's ts is reset on every update, which happens on pings too.

Author: PeterCxy

integration_test/connection_pool/max_lifetime_test.exs

@@ -4,6 +4,51 @@ defmodule MaxLifetimeTest do
   alias TestPool, as: P
   alias TestAgent, as: A
 
+  defmodule ManyPingsConnection do
+    def connect(opts) do
+      {:ok, %{parent: Keyword.fetch!(opts, :parent), pings: 0}}
+    end
+
+    def disconnect(err, state) do
+      send(state.parent, {:max_lifetime_disconnect, err, state.pings})
+      :ok
+    end
+
+    def checkout(state), do: {:ok, state}
+    def checkin(state), do: {:ok, state}
+
+    def ping(state) do
+      state = %{state | pings: state.pings + 1}
+      send(state.parent, {:ping, state.pings})
+      {:ok, state}
+    end
+  end
+
+  test "disconnects and reconnects when idle ping fires before max_lifetime" do
+    opts = [
+      parent: self(),
+      pool: DBConnection.ConnectionPool,
+      pool_size: 1,
+      connection_listeners: [self()],
+      max_lifetime: 200..200,
+      idle_interval: 50,
+      backoff_min: 10
+    ]
+
+    {:ok, _pool} = DBConnection.start_link(ManyPingsConnection, opts)
+
+    assert_receive {:connected, conn}
+    assert_receive {:ping, 1}
+
+    assert_receive {:max_lifetime_disconnect,
+                    %DBConnection.ConnectionError{message: "max_lifetime exceeded"}, pings},
+                   2_000
+
+    assert pings > 0
+    assert_receive {:disconnected, ^conn}
+    assert_receive {:connected, ^conn}
+  end
+
   test "disconnects and reconnects when idle ping fires after max_lifetime" do
     stack = [
       {:ok, :state},

lib/db_connection/connection.ex

@@ -61,6 +61,7 @@ defmodule DBConnection.Connection do
       pool: pool,
       tag: tag,
       timer: nil,
+      connected_at: nil,
       backoff: Backoff.new(opts),
       connection_listeners: Keyword.get(opts, :connection_listeners, []),
       after_connect: Keyword.get(opts, :after_connect),
@@ -85,14 +86,24 @@ defmodule DBConnection.Connection do
     else
       {:ok, state} when after_connect != nil ->
         ref = make_ref()
+        connected_at = System.monotonic_time()
         :gen_statem.cast(self(), {:after_connect, ref})
-        {:keep_state, %{s | state: state, client: {ref, :connect}}}
+        {:keep_state, %{s | state: state, client: {ref, :connect}, connected_at: connected_at}}
 
       {:ok, state} ->
         backoff = backoff && Backoff.reset(backoff)
         ref = make_ref()
+        connected_at = System.monotonic_time()
         :gen_statem.cast(self(), {:connected, ref})
-        {:keep_state, %{s | state: state, client: {ref, :connect}, backoff: backoff}}
+
+        {:keep_state,
+         %{
+           s
+           | state: state,
+             client: {ref, :connect},
+             backoff: backoff,
+             connected_at: connected_at
+         }}
 
       {:error, err} when is_nil(backoff) ->
         Logger.error(
@@ -154,7 +165,7 @@ defmodule DBConnection.Connection do
     demonitor(client)
     cancel_timer(timer)
     :ok = apply(mod, :disconnect, [err, state])
-    s = %{s | state: nil, client: :closed, timer: nil}
+    s = %{s | state: nil, client: :closed, timer: nil, connected_at: nil}
 
     notify_connection_listeners(:disconnected, s)
 
@@ -470,8 +481,8 @@ defmodule DBConnection.Connection do
     pool_update(state, %{s | client: nil, backoff: backoff})
   end
 
-  defp pool_update(state, %{pool: pool, tag: tag, mod: mod} = s) do
-    case Holder.update(pool, tag, mod, state) do
+  defp pool_update(state, %{pool: pool, tag: tag, mod: mod, connected_at: connected_at} = s) do
+    case Holder.update(pool, tag, mod, state, connected_at) do
       {:ok, ref} ->
         {:keep_state, %{s | client: {ref, :pool}, state: state}, :hibernate}
 

lib/db_connection/holder.ex

@@ -8,7 +8,16 @@ defmodule DBConnection.Holder do
   @timeout 15000
   @time_unit 1000
 
-  Record.defrecord(:conn, [:connection, :module, :state, :lock, :ts, deadline: nil, status: :ok])
+  Record.defrecord(:conn, [
+    :connection,
+    :module,
+    :state,
+    :lock,
+    :connected_at,
+    deadline: nil,
+    status: :ok
+  ])
+
   Record.defrecord(:pool_ref, [:pool, :reference, :deadline, :holder, :lock])
 
   @type t :: :ets.tid()
@@ -17,20 +26,22 @@ defmodule DBConnection.Holder do
   ## Holder API
 
   @spec new(pid, reference, module, term) :: t
-  def new(pool, ref, mod, state) do
+  @spec new(pid, reference, module, term, integer) :: t
+  def new(pool, ref, mod, state, connected_at \\ System.monotonic_time()) do
     # Insert before setting heir so that pool can't receive empty table
     holder = :ets.new(__MODULE__, [:public, :ordered_set, decentralized_counters: true])
 
-    conn = conn(connection: self(), module: mod, state: state, ts: System.monotonic_time())
+    conn = conn(connection: self(), module: mod, state: state, connected_at: connected_at)
     true = :ets.insert_new(holder, conn)
 
     :ets.setopts(holder, {:heir, pool, ref})
     holder
   end
 
   @spec update(pid, reference, module, term) :: {:ok, t} | :error
-  def update(pool, ref, mod, state) do
-    holder = new(pool, ref, mod, state)
+  @spec update(pid, reference, module, term, integer) :: {:ok, t} | :error
+  def update(pool, ref, mod, state, connected_at \\ System.monotonic_time()) do
+    holder = new(pool, ref, mod, state, connected_at)
 
     try do
       :ets.give_away(holder, pool, {:checkin, ref, System.monotonic_time()})
@@ -244,7 +255,7 @@ defmodule DBConnection.Holder do
   @spec maybe_disconnect(t, integer, non_neg_integer, {integer, non_neg_integer} | nil) ::
           boolean()
   def maybe_disconnect(holder, start, interval_ms, lifetime) do
-    ts = :ets.lookup_element(holder, :conn, conn(:ts) + 1)
+    ts = :ets.lookup_element(holder, :conn, conn(:connected_at) + 1)
 
     disconnect_all_reason(start, interval_ms, ts, holder) ||
       max_lifetime_reason(lifetime, ts, holder)