Refactor: Split CallbackTracingServer to separate gen servers (#323)

* Split gen server

* Add tests for EtsTableServer

* Make EtsTableServer mockable

* Add more tests

Author: hhubert6

lib/live_debugger.ex

@@ -34,7 +34,8 @@ defmodule LiveDebugger do
       else
         children ++
           [
-            {LiveDebugger.GenServers.CallbackTracingServer, []}
+            {LiveDebugger.GenServers.CallbackTracingServer, []},
+            {LiveDebugger.GenServers.EtsTableServer, []}
           ]
       end
 

lib/live_debugger/gen_servers/callback_tracing_server.ex

@@ -1,6 +1,6 @@
 defmodule LiveDebugger.GenServers.CallbackTracingServer do
   @moduledoc """
-  This gen_server is responsible for tracing callbacks and managing ETS tables.
+  This gen_server is responsible for tracing callbacks.
   """
 
   use GenServer
@@ -15,30 +15,10 @@ defmodule LiveDebugger.GenServers.CallbackTracingServer do
   alias LiveDebugger.Utils.Callbacks, as: CallbackUtils
   alias LiveDebugger.Utils.PubSub, as: PubSubUtils
 
-  @ets_table_name :lvdbg_traces
   @callback_functions CallbackUtils.callbacks_functions()
 
-  @type table_refs() :: %{pid() => :ets.table()}
-
   ## API
 
-  @doc """
-  Returns ETS table reference.
-  It creates table if none is associated with given pid
-  """
-  @spec table!(pid :: pid()) :: :ets.table()
-  def table!(pid) when is_pid(pid) do
-    GenServer.call(__MODULE__, {:get_or_create_table, pid}, 1000)
-  end
-
-  @doc """
-  If table for given `pid` exists it deletes it from ETS.
-  """
-  @spec delete_table!(pid :: pid()) :: :ok
-  def delete_table!(pid) when is_pid(pid) do
-    GenServer.call(__MODULE__, {:delete_table, pid}, 1000)
-  end
-
   @doc """
   Checks if GenServer has been loaded
   """
@@ -63,7 +43,7 @@ defmodule LiveDebugger.GenServers.CallbackTracingServer do
   end
 
   @impl true
-  def handle_info(:setup_tracing, table_refs) do
+  def handle_info(:setup_tracing, state) do
     Dbg.tracer(:process, {&handle_trace/2, 0})
     Dbg.p(:all, :c)
 
@@ -84,61 +64,14 @@ defmodule LiveDebugger.GenServers.CallbackTracingServer do
     # We trace it to refresh the components tree
     Dbg.tp({Phoenix.LiveView.Diff, :delete_component, 2}, [])
 
-    {:noreply, table_refs}
-  end
-
-  @impl true
-  def handle_info({:DOWN, _, :process, closed_pid, _}, table_refs) do
-    {_, table_refs} = delete_ets_table(closed_pid, table_refs)
-
-    closed_pid
-    |> PubSubUtils.process_status_topic()
-    |> PubSubUtils.broadcast({:process_status, :dead})
-
-    {:noreply, table_refs}
-  end
-
-  @impl true
-  def handle_call({:get_or_create_table, pid}, _from, table_refs) do
-    case Map.get(table_refs, pid) do
-      nil ->
-        ref = create_ets_table()
-        Process.monitor(pid)
-        {:reply, ref, Map.put(table_refs, pid, ref)}
-
-      ref ->
-        {:reply, ref, table_refs}
-    end
-  end
-
-  @impl true
-  def handle_call({:delete_table, pid}, _from, table_refs) do
-    {_, table_refs} = delete_ets_table(pid, table_refs)
-    {:reply, :ok, table_refs}
+    {:noreply, state}
   end
 
   @impl true
   def handle_call(:ping, _from, state) do
     {:reply, :ok, state}
   end
 
-  @spec create_ets_table() :: :ets.table()
-  defp create_ets_table() do
-    :ets.new(@ets_table_name, [:ordered_set, :public])
-  end
-
-  @spec delete_ets_table(pid(), table_refs()) :: {boolean(), table_refs()}
-  defp delete_ets_table(pid, table_refs) do
-    case Map.pop(table_refs, pid) do
-      {nil, table_refs} ->
-        {false, table_refs}
-
-      {ref, updated_table_refs} ->
-        :ets.delete(ref)
-        {true, updated_table_refs}
-    end
-  end
-
   # This handler is heavy because of fetching state and we do not care for order because it is not displayed to user
   # Because of that we do it asynchronously to speed up tracer a bit
   # We do not persist this trace because it is not displayed to user

lib/live_debugger/gen_servers/ets_table_server.ex

@@ -0,0 +1,114 @@
+defmodule LiveDebugger.GenServers.EtsTableServer do
+  @moduledoc """
+  This gen_server is responsible for managing ETS tables.
+  """
+
+  use GenServer
+
+  alias LiveDebugger.Utils.PubSub, as: PubSubUtils
+
+  @ets_table_name :lvdbg_traces
+
+  @type table_refs() :: %{pid() => :ets.table()}
+
+  ## API
+
+  @callback table!(pid :: pid()) :: :ets.table()
+  @callback delete_table!(pid :: pid()) :: :ok
+
+  @doc """
+  Returns ETS table reference.
+  It creates table if none is associated with given pid
+  """
+  @spec table!(pid :: pid()) :: :ets.table()
+  def table!(pid) when is_pid(pid) do
+    impl().table!(pid)
+  end
+
+  @doc """
+  If table for given `pid` exists it deletes it from ETS.
+  """
+  @spec delete_table!(pid :: pid()) :: :ok
+  def delete_table!(pid) when is_pid(pid) do
+    impl().delete_table!(pid)
+  end
+
+  def impl() do
+    Application.get_env(:live_debugger, :ets_table_server, __MODULE__.Impl)
+  end
+
+  defmodule Impl do
+    @moduledoc false
+    @behaviour LiveDebugger.GenServers.EtsTableServer
+    @server_module LiveDebugger.GenServers.EtsTableServer
+
+    @impl true
+    def table!(pid) do
+      GenServer.call(@server_module, {:get_or_create_table, pid}, 1000)
+    end
+
+    @impl true
+    def delete_table!(pid) do
+      GenServer.call(@server_module, {:delete_table, pid}, 1000)
+    end
+  end
+
+  ## GenServer
+
+  @doc false
+  def start_link(args \\ []) do
+    GenServer.start_link(__MODULE__, args, name: __MODULE__)
+  end
+
+  @impl true
+  def init(_args) do
+    {:ok, %{}}
+  end
+
+  @impl true
+  def handle_info({:DOWN, _, :process, closed_pid, _}, table_refs) do
+    {_, table_refs} = delete_ets_table(closed_pid, table_refs)
+
+    closed_pid
+    |> PubSubUtils.process_status_topic()
+    |> PubSubUtils.broadcast({:process_status, :dead})
+
+    {:noreply, table_refs}
+  end
+
+  @impl true
+  def handle_call({:get_or_create_table, pid}, _from, table_refs) do
+    case Map.get(table_refs, pid) do
+      nil ->
+        ref = create_ets_table()
+        Process.monitor(pid)
+        {:reply, ref, Map.put(table_refs, pid, ref)}
+
+      ref ->
+        {:reply, ref, table_refs}
+    end
+  end
+
+  @impl true
+  def handle_call({:delete_table, pid}, _from, table_refs) do
+    {_, table_refs} = delete_ets_table(pid, table_refs)
+    {:reply, :ok, table_refs}
+  end
+
+  @spec create_ets_table() :: :ets.table()
+  defp create_ets_table() do
+    :ets.new(@ets_table_name, [:ordered_set, :public])
+  end
+
+  @spec delete_ets_table(pid(), table_refs()) :: {boolean(), table_refs()}
+  defp delete_ets_table(pid, table_refs) do
+    case Map.pop(table_refs, pid) do
+      {nil, table_refs} ->
+        {false, table_refs}
+
+      {ref, updated_table_refs} ->
+        :ets.delete(ref)
+        {true, updated_table_refs}
+    end
+  end
+end

lib/live_debugger/services/trace_service.ex

@@ -1,12 +1,12 @@
 defmodule LiveDebugger.Services.TraceService do
   @moduledoc """
   This module is responsible for accessing traces from ETS.
-  It uses calls to `CallbackTracingServer` to get proper table reference.
+  It uses calls to `EtsTableServer` to get proper table reference.
   """
 
   alias LiveDebugger.Structs.Trace
   alias LiveDebugger.CommonTypes
-  alias LiveDebugger.GenServers.CallbackTracingServer
+  alias LiveDebugger.GenServers.EtsTableServer
   alias Phoenix.LiveComponent.CID
 
   @default_limit 100
@@ -15,7 +15,7 @@ defmodule LiveDebugger.Services.TraceService do
   @type ets_continuation :: term()
   @typedoc """
   Pid is used to store mapping to table references.
-  It identifies ETS tables managed by CallbackTracingServer
+  It identifies ETS tables managed by EtsTableServer.
   """
   @type ets_table_id() :: pid()
 
@@ -149,6 +149,6 @@ defmodule LiveDebugger.Services.TraceService do
 
   @spec ets_table!(pid :: ets_table_id()) :: :ets.table()
   defp ets_table!(pid) when is_pid(pid) do
-    CallbackTracingServer.table!(pid)
+    EtsTableServer.table!(pid)
   end
 end

test/gen_servers/ets_table_server_test.exs

@@ -0,0 +1,122 @@
+defmodule LiveDebugger.GenServers.EtsTableServerTest do
+  @moduledoc false
+  use ExUnit.Case, async: true
+
+  alias LiveDebugger.Utils.PubSub, as: PubSubUtils
+  alias LiveDebugger.GenServers.EtsTableServer
+
+  test "start_link/1" do
+    assert {:ok, _pid} = EtsTableServer.start_link()
+    GenServer.stop(EtsTableServer)
+  end
+
+  test "init/1" do
+    assert {:ok, %{}} = EtsTableServer.init([])
+  end
+
+  describe "gen server api" do
+    test "table!/1" do
+      pid = :c.pid(0, 0, 1)
+
+      LiveDebugger.MockEtsTableServer
+      |> Mox.expect(:table!, fn ^pid -> :some_ref end)
+
+      assert :some_ref = EtsTableServer.table!(pid)
+    end
+
+    test "delete_table!/1" do
+      pid = :c.pid(0, 0, 1)
+
+      LiveDebugger.MockEtsTableServer
+      |> Mox.expect(:delete_table!, fn ^pid -> :ok end)
+
+      assert :ok = EtsTableServer.delete_table!(pid)
+    end
+  end
+
+  describe "handle_info/2" do
+    test "deletes table ref after process down" do
+      pid = :c.pid(0, 0, 1)
+      ref = :ets.new(:test_table, [])
+
+      other_pid = :c.pid(0, 0, 2)
+      other_ref = :ets.new(:test_table, [])
+
+      table_refs = %{pid => ref, other_pid => other_ref}
+
+      topic = PubSubUtils.process_status_topic(pid)
+
+      LiveDebugger.MockPubSubUtils
+      |> Mox.expect(:broadcast, fn ^topic, {:process_status, :dead} -> :ok end)
+
+      assert {:noreply, new_table_refs} =
+               EtsTableServer.handle_info({:DOWN, :_, :process, pid, :_}, table_refs)
+
+      assert :undefined == :ets.info(ref)
+      assert nil == Map.get(new_table_refs, pid)
+
+      assert [{:id, ^other_ref} | _] = :ets.info(other_ref)
+      assert other_ref == Map.get(new_table_refs, other_pid)
+    end
+  end
+
+  describe "handle_call/3" do
+    test "deletes table on event {:delete_table, pid}" do
+      pid = :c.pid(0, 0, 1)
+      ref = :ets.new(:test_table, [])
+
+      other_pid = :c.pid(0, 0, 2)
+      other_ref = :ets.new(:test_table, [])
+
+      table_refs = %{pid => ref, other_pid => other_ref}
+
+      assert {:reply, :ok, new_table_refs} =
+               EtsTableServer.handle_call({:delete_table, pid}, self(), table_refs)
+
+      assert :undefined == :ets.info(ref)
+      assert nil == Map.get(new_table_refs, pid)
+
+      assert [{:id, ^other_ref} | _] = :ets.info(other_ref)
+      assert other_ref == Map.get(new_table_refs, other_pid)
+    end
+
+    test "ignores delete table on event {:delete_table, pid} when table does not exist" do
+      pid = :c.pid(0, 0, 1)
+      ref = :ets.new(:test_table, [])
+
+      other_pid = :c.pid(0, 0, 2)
+
+      table_refs = %{pid => ref}
+
+      assert {:reply, :ok, new_table_refs} =
+               EtsTableServer.handle_call({:delete_table, other_pid}, self(), table_refs)
+
+      assert [{:id, ^ref} | _] = :ets.info(ref)
+      assert ref == Map.get(new_table_refs, pid)
+
+      assert nil == Map.get(new_table_refs, other_pid)
+    end
+
+    test "creates table on event {:get_or_create_table, pid}" do
+      pid = :c.pid(0, 0, 1)
+      table_refs = %{}
+
+      assert {:reply, ref, new_table_refs} =
+               EtsTableServer.handle_call({:get_or_create_table, pid}, self(), table_refs)
+
+      assert [{:id, ^ref} | _] = :ets.info(ref)
+      assert ref == Map.get(new_table_refs, pid)
+    end
+
+    test "returns existing table on event {:get_or_create_table, pid}" do
+      pid = :c.pid(0, 0, 1)
+      ref = :ets.new(:test_table, [])
+      table_refs = %{pid => ref}
+
+      assert {:reply, ^ref, new_table_refs} =
+               EtsTableServer.handle_call({:get_or_create_table, pid}, self(), table_refs)
+
+      assert ref == Map.get(new_table_refs, pid)
+    end
+  end
+end