From bcf2584cc37c25be4ef84227f25cb10499b99833 Mon Sep 17 00:00:00 2001
From: Eric Avdey <eiri@eiri.ca>
Date: Thu, 7 May 2020 14:15:08 -0300
Subject: Address comments

---
 src/aegis/src/aegis_server.erl | 82 +++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 36 deletions(-)

diff --git a/src/aegis/src/aegis_server.erl b/src/aegis/src/aegis_server.erl
index 9596b98c7..12d0afd4e 100644
--- a/src/aegis/src/aegis_server.erl
+++ b/src/aegis/src/aegis_server.erl
@@ -88,10 +88,8 @@ encrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
         uuid := UUID
     } = Db,
 
-    Now = fabric2_util:now(sec),
-
-    case ets:lookup(?KEY_CHECK, UUID) of
-        [{UUID, ExpiresAt}] when ExpiresAt >= Now ->
+    case is_key_present(UUID) of
+        true ->
             case gen_server:call(?MODULE, {encrypt, Db, Key, Value}) of
                 CipherText when is_binary(CipherText) ->
                     CipherText;
@@ -101,7 +99,7 @@ encrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
                 {error, Reason} ->
                     erlang:error(Reason)
             end;
-        _ ->
+        false ->
             process_flag(sensitive, true),
 
             {ok, DbKey} = do_open_db(Db),
@@ -115,10 +113,8 @@ decrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
         uuid := UUID
     } = Db,
 
-    Now = fabric2_util:now(sec),
-
-    case ets:lookup(?KEY_CHECK, UUID) of
-        [{UUID, ExpiresAt}] when ExpiresAt >= Now ->
+    case is_key_present(UUID) of
+        true ->
             case gen_server:call(?MODULE, {decrypt, Db, Key, Value}) of
                 PlainText when is_binary(PlainText) ->
                     PlainText;
@@ -128,7 +124,7 @@ decrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
                 {error, Reason} ->
                     erlang:error(Reason)
             end;
-        _ ->
+        false ->
             process_flag(sensitive, true),
 
             {ok, DbKey} = do_open_db(Db),
@@ -159,7 +155,13 @@ terminate(_Reason, _St) ->
     ok.
 
 
-handle_call({insert_key, UUID, DbKey}, _From, St) ->
+handle_call({insert_key, UUID, DbKey}, _From, #{cache := Cache} = St) ->
+    case ets:lookup(Cache, UUID) of
+        [#entry{uuid = UUID} = Entry] ->
+            delete(St, Entry);
+        [] ->
+            ok
+    end,
     NewSt = insert(St, UUID, DbKey),
     {reply, ok, NewSt, ?TIMEOUT};
 
@@ -216,7 +218,8 @@ handle_cast(_Msg, St) ->
 
 handle_info(maybe_remove_expired, St) ->
     remove_expired_entries(St),
-    CheckInterval = timer:seconds(expiration_check_interval()),
+    CheckInterval = erlang:convert_time_unit(
+        expiration_check_interval(), second, millisecond),
     erlang:send_after(CheckInterval, self(), maybe_remove_expired),
     {noreply, St};
 
@@ -276,6 +279,15 @@ do_decrypt(DbKey, #{uuid := UUID}, Key, Value) ->
     end.
 
 
+is_key_present(UUID) ->
+    Now = fabric2_util:now(sec),
+
+    case ets:lookup(?KEY_CHECK, UUID) of
+        [{UUID, ExpiresAt}] when ExpiresAt >= Now -> true;
+        _ -> false
+    end.
+
+
 %% cache functions
 
 insert(St, UUID, DbKey) ->
@@ -300,7 +312,17 @@ insert(St, UUID, DbKey) ->
     true = ets:insert_new(ByAccess, Entry),
     true = ets:insert(?KEY_CHECK, {UUID, ExpiresAt}),
 
-    maybe_evict_old_entries(St),
+    CacheLimit = cache_limit(),
+    CacheSize = ets:info(Cache, size),
+
+    case CacheSize > CacheLimit of
+        true ->
+            LRUKey = ets:first(ByAccess),
+            [LRUEntry] = ets:lookup(ByAccess, LRUKey),
+            delete(St, LRUEntry);
+        false ->
+            ok
+    end,
 
     St#{counter := Counter + 1}.
 
@@ -315,6 +337,17 @@ lookup(#{cache := Cache}, UUID) ->
     end.
 
 
+delete(St, #entry{uuid = UUID} = Entry) ->
+    #{
+        cache := Cache,
+        by_access := ByAccess
+    } = St,
+
+    true = ets:delete(?KEY_CHECK, UUID),
+    true = ets:delete_object(Cache, Entry),
+    true = ets:delete_object(ByAccess, Entry).
+
+
 maybe_bump_last_accessed(#entry{last_accessed = LastAccessed} = Entry) ->
     case fabric2_util:now(sec) > LastAccessed + ?LAST_ACCESSED_INACTIVITY_SEC of
         true ->
@@ -347,29 +380,6 @@ bump_last_accessed(St, UUID) ->
     St#{counter := Counter + 1}.
 
 
-maybe_evict_old_entries(#{cache := Cache} = St) ->
-    CacheLimit = cache_limit(),
-    CacheSize = ets:info(Cache, size),
-    evict_old_entries(St, CacheSize - CacheLimit).
-
-
-evict_old_entries(St, N) when N > 0 ->
-    #{
-        cache := Cache,
-        by_access := ByAccess
-    } = St,
-
-    OldestKey = ets:first(ByAccess),
-    [#entry{uuid = UUID}] = ets:lookup(ByAccess, OldestKey),
-    true = ets:delete(?KEY_CHECK, UUID),
-    true = ets:delete(Cache, UUID),
-    true = ets:delete(ByAccess, OldestKey),
-    evict_old_entries(St, N - 1);
-
-evict_old_entries(_St, _) ->
-    ok.
-
-
 remove_expired_entries(St) ->
     #{
         cache := Cache,
-- 
cgit v1.2.1