From 76436256eacad6cb29f5686b4b57899c4dfb6751 Mon Sep 17 00:00:00 2001
From: Maximilien Rothier Bautzer <m.rothierbautzer@gmail.com>
Date: Thu, 2 Dec 2021 15:04:19 +0000
Subject: [PATCH] Use ETS cache to store features rather than Genserver state

---
 lib/unleash/cache.ex        | 78 +++++++++++++++++++++++++++++++++++++
 lib/unleash/repo.ex         | 73 +++++++++++++++++-----------------
 test/unleash/cache_test.exs | 53 +++++++++++++++++++++++++
 3 files changed, 168 insertions(+), 36 deletions(-)
 create mode 100644 lib/unleash/cache.ex
 create mode 100644 test/unleash/cache_test.exs

diff --git a/lib/unleash/cache.ex b/lib/unleash/cache.ex
new file mode 100644
index 0000000..eb73c82
--- /dev/null
+++ b/lib/unleash/cache.ex
@@ -0,0 +1,78 @@
+defmodule Unleash.Cache do
+  @moduledoc """
+  This module is a cache backed by an ETS table. We use it to allow for multiple
+  threads to read the feature flag values concurrently on top of minimizing
+  network calls
+  """
+
+  @cache_table_name :unleash_cache
+
+  def cache_table_name, do: @cache_table_name
+
+  @doc """
+  Will create a new ETS table named `:unleash_cache`
+  """
+  def init(existing_features \\ [], table_name \\ @cache_table_name) do
+    :ets.new(table_name, [:named_table, read_concurrency: true])
+
+    upsert_features(existing_features, table_name)
+  end
+
+  @doc """
+  Will return all values currently stored in the cache
+  """
+  def get_all_feature_names(table_name \\ @cache_table_name) do
+    features = :ets.tab2list(table_name)
+
+    Enum.map(features, fn {name, _feature} ->
+      name
+    end)
+  end
+
+  @doc """
+  Will return all features stored in the cache
+  """
+  def get_features(table_name \\ @cache_table_name) do
+    features = :ets.tab2list(table_name)
+
+    Enum.map(features, fn {_name, feature} ->
+      feature
+    end)
+  end
+
+  @doc """
+  Will return the feature for the given name stored in the cache
+  """
+  def get_feature(name, table_name \\ @cache_table_name)
+
+  def get_feature(name, table_name) when is_binary(name) do
+    case :ets.lookup(table_name, name) do
+      [{^name, feature}] -> feature
+      [] -> nil
+    end
+  end
+
+  def get_feature(name, table_name) when is_atom(name),
+    do: get_feature(Atom.to_string(name), table_name)
+
+  @doc """
+  Will upsert (create or update) the given features in the cache
+
+  This will clear the existing peristed features to prevent stale reads
+  """
+  def upsert_features(features, table_name \\ @cache_table_name) do
+    :ets.delete_all_objects(table_name)
+
+    Enum.each(features, fn feature ->
+      upsert_feature(feature.name, feature, table_name)
+    end)
+  end
+
+  defp upsert_feature(name, value, table_name) when is_binary(name) do
+    :ets.insert(table_name, {name, value})
+  end
+
+  defp upsert_feature(name, value, table_name) when is_atom(name) do
+    upsert_feature(Atom.to_string(name), value, table_name)
+  end
+end
diff --git a/lib/unleash/repo.ex b/lib/unleash/repo.ex
index f4d833a..d2afb40 100644
--- a/lib/unleash/repo.ex
+++ b/lib/unleash/repo.ex
@@ -1,23 +1,36 @@
 defmodule Unleash.Repo do
-  @moduledoc false
+  @moduledoc """
+  This genserver polls the unleash service each time the given interval has
+  elapsed, refreshing both our local ETS cache and the backup state file if the
+  flag state has diverged.
+
+  The following configuration values are used:
+
+  Config.features_period(): polling interval - default 15 seconds
+  Config.retries(): number of time the call to refresh values is allowed to retry - default -1 (0)
+  """
+
   use GenServer
   require Logger
 
+  alias Unleash.Cache
   alias Unleash.Config
   alias Unleash.Features
 
   def init(%Features{} = features) do
-    {:ok, features}
+    Cache.init(features.features)
+    {:ok, []}
   end
 
   def init(_) do
-    {:ok, %Features{}}
+    Cache.init()
+    {:ok, []}
   end
 
   def start_link(state) do
     {:ok, pid} = GenServer.start_link(__MODULE__, state, name: Unleash.Repo)
 
-    unless Config.test? do
+    unless Config.test?() do
       initialize()
     end
 
@@ -25,47 +38,40 @@ defmodule Unleash.Repo do
   end
 
   def get_feature(name) do
-    GenServer.call(Unleash.Repo, {:get_feature, name})
+    Cache.get_feature(name)
   end
 
   def get_all_feature_names do
-    GenServer.call(Unleash.Repo, {:get_all_feature_names})
-  end
-
-  def handle_call({:get_feature, name}, _from, state) do
-    feature = Features.get_feature(state, name)
-
-    {:reply, feature, state}
-  end
-
-  def handle_call({:get_all_feature_names}, _from, state) do
-    names = Features.get_all_feature_names(state)
-    {:reply, names, state}
+    Cache.get_all_feature_names()
   end
 
   def handle_info({:initialize, etag, retries}, state) do
     if retries > 0 or retries <= -1 do
+      cached_features = %Features{features: Cache.get_features()}
+
       {etag, response} =
         case Unleash.Config.client().features(etag) do
-          :cached -> {etag, state}
+          :cached -> {etag, cached_features}
           x -> x
         end
 
-      features =
+      remote_features =
         case response do
           {:error, _} ->
-            state
-            |> read_state()
+            cached_features
+            |> read_file_state()
             |> schedule_features(etag, retries - 1)
 
           f ->
             schedule_features(f, etag)
         end
 
-      if features === state do
+      if remote_features === cached_features do
         {:noreply, state}
       else
-        write_state(features)
+        Cache.upsert_features(remote_features.features)
+        write_file_state(remote_features)
+        {:noreply, state}
       end
     else
       Logger.debug(fn ->
@@ -83,26 +89,23 @@ defmodule Unleash.Repo do
     {:noreply, state}
   end
 
-  defp read_state(%Features{features: []} = state) do
+  defp read_file_state(%Features{features: []} = cached_features) do
     if File.exists?(Config.backup_file()) do
       Config.backup_file()
       |> File.read!()
       |> Jason.decode!()
       |> Features.from_map!()
     else
-      state
+      cached_features
     end
   end
 
-  defp read_state(state), do: state
+  defp read_file_state(cached_features), do: cached_features
 
-  defp write_state(state) do
-    if not File.dir?(Config.backup_dir()) do
-      Config.backup_dir()
-      |> File.mkdir_p!()
-    end
+  defp write_file_state(features) do
+    :ok = File.mkdir_p(Config.backup_dir())
 
-    content = Jason.encode_to_iodata!(state)
+    content = Jason.encode_to_iodata!(features)
 
     Config.backup_file()
     |> File.write!(content)
@@ -110,15 +113,13 @@ defmodule Unleash.Repo do
     Logger.debug(fn ->
       ["Wrote ", content, " to file ", Config.backup_file()]
     end)
-
-    {:noreply, state}
   end
 
   defp initialize do
     Process.send(Unleash.Repo, {:initialize, nil, Config.retries()}, [])
   end
 
-  defp schedule_features(state, etag, retries \\ Config.retries()) do
+  defp schedule_features(features, etag, retries \\ Config.retries()) do
     Logger.debug(fn ->
       retries_log =
         if retries >= 0 do
@@ -131,6 +132,6 @@ defmodule Unleash.Repo do
     end)
 
     Process.send_after(self(), {:initialize, etag, retries}, Config.features_period())
-    state
+    features
   end
 end
diff --git a/test/unleash/cache_test.exs b/test/unleash/cache_test.exs
new file mode 100644
index 0000000..90b4220
--- /dev/null
+++ b/test/unleash/cache_test.exs
@@ -0,0 +1,53 @@
+defmodule Unleash.CacheTest do
+  @moduledoc false
+  use ExUnit.Case, async: true
+
+  alias Unleash.Cache
+  alias Unleash.Feature
+
+  @existing_feature %Feature{name: "exists"}
+  @existing_features [%Feature{name: "exists"}]
+
+  @new_feature %Feature{name: "new"}
+  @new_features [%Feature{name: "new"}]
+
+  setup context do
+    Cache.init([], context.test)
+
+    assert :ok == Cache.upsert_features(@existing_features, context.test)
+
+    {:ok, table_name: context.test}
+  end
+
+  describe "get_feature/1" do
+    test "get_feature succeeds if the feature name is present", %{table_name: table_name} do
+      assert @existing_feature == Cache.get_feature(@existing_feature.name, table_name)
+    end
+
+    test "get_feature fails if the key does not exist", %{table_name: table_name} do
+      assert nil == Cache.get_feature(@new_feature.name, table_name)
+    end
+  end
+
+  describe "get_all_feature_names/1" do
+    test "get_all_feature_names succeeds", %{
+      table_name: table_name
+    } do
+      assert :ok == Cache.upsert_features(@new_features ++ @existing_features, table_name)
+
+      assert [@existing_feature.name, @new_feature.name] ==
+               Cache.get_all_feature_names(table_name)
+    end
+  end
+
+  describe "upsert_features/2" do
+    test "upsert overwrites existing features", %{
+      table_name: table_name
+    } do
+      assert :ok == Cache.upsert_features(@new_features, table_name)
+
+      assert nil == Cache.get_feature(@existing_feature.name, table_name)
+      assert @new_feature == Cache.get_feature(@new_feature.name, table_name)
+    end
+  end
+end