fix: Handle connection errors during read operations. (#163)

This PR adds error handling for read operations from the redis
persistent store. This is an incremental step in addressing #161.

In the case we cannot read from redis, we will treat that as a flag not
being found. If we cannot read from redis when doing an initialization
check, then we will consider the store not to have been initialized.

This addresses the primary issues with daemon mode, but it doesn't
address persistent store errors in non-daemon mode, or other cases that
could cause the redis process to exit. Subsequent PRs will address these
issues.

The detailed variation interface does not report why a flag cannot be
found, and isn't the correct avenue for doing so, and as such we may
have to consider supporting the data store status interface. This is
also not tackled in this PR.

Author: kinyoklion

Makefile

@@ -22,8 +22,8 @@ run:
 doc:
 	@$(REBAR3) edoc
 
-tests:
-	docker run --name ld-test-redis -p 6379:6379 -d redis
+tests: clean-redis
+	docker run --name ld-test-redis -p 6379:6379 -d redis:7.2
 	@$(REBAR3) ct --dir="test,test-redis" --logdir logs/ct
 	docker rm --force ld-test-redis
 
@@ -40,7 +40,7 @@ tls-tests:
 
 #This is for local debugging if your tests fail and the Redis Docker container is not torn down properly
 clean-redis:
-	docker rm --force ld-test-redis
+	docker rm --force ld-test-redis 2> /dev/null || true
 
 colon := :
 build-contract-tests:

rebar.config

@@ -14,7 +14,7 @@
     {test, [
         {deps, [
             {bookish_spork, "0.3.5"},
-            {cowboy, "2.8.0"},
+            {cowboy, "2.10.0"},
             {meck, "0.9.2"}
         ]},
         {extra_src_dirs, [{"test", [{recursive, true}]}]}

src/ldclient_storage_redis_server.erl

@@ -236,11 +236,16 @@ empty_bucket(true, Bucket, Client, Prefix) ->
 all_items(false, Bucket, _Client, _Prefix) ->
     {error, bucket_not_found, "Redis hash " ++ atom_to_list(Bucket) ++ " does not exist."};
 all_items(true, Bucket, Client, Prefix) ->
-    {ok, Values} = eredis:q(Client, ["HGETALL", bucket_name(Prefix, Bucket)]),
-    NullFilter = [<<"null">>],
-    NewValues = lists:filter(fun(Elem) ->
-        not lists:member(Elem, NullFilter) end, Values), %This removes the initial null key and value
-    pairs(NewValues, Bucket).
+    case eredis:q(Client, ["HGETALL", bucket_name(Prefix, Bucket)]) of
+        {ok, Values} ->
+            NullFilter = [<<"null">>],
+            NewValues = lists:filter(fun(Elem) ->
+                not lists:member(Elem, NullFilter) end, Values), %This removes the initial null key and value
+            pairs(NewValues, Bucket);
+        {error, Reason} ->
+            error_logger:error_msg("Redis error listing all items in bucket ~p: ~s", [Bucket, format_error(Reason)]),
+            []
+    end.
 
 pairs([A, B | L], Bucket) ->
     Decoded = jsx:decode(B, [return_maps]),
@@ -265,19 +270,24 @@ pairs([], _Bucket) -> [].
 lookup_key(false, _Key, Bucket, _Client, _Prefix) ->
     {error, bucket_not_found, "Redis hash " ++ atom_to_list(Bucket) ++ " does not exist."};
 lookup_key(true, Key, Bucket, Client, Prefix) ->
-    {ok, Value} = eredis:q(Client, ["HGET", bucket_name(Prefix, Bucket), Key]),
-    if
-        (Value == undefined) -> [];
-        true ->
-            Decoded = jsx:decode(Value, [return_maps]),
+    case eredis:q(Client, ["HGET", bucket_name(Prefix, Bucket), Key]) of
+        {ok, Value} ->
             if
-                Bucket == features ->
-                    Parsed = ldclient_flag:new(Decoded),
-                    [{Key, Parsed}];
-                Bucket == segments ->
-                    Parsed = ldclient_segment:new(Decoded),
-                    [{Key, Parsed}]
-            end
+                (Value == undefined) -> [];
+                true ->
+                    Decoded = jsx:decode(Value, [return_maps]),
+                    if
+                        Bucket == features ->
+                            Parsed = ldclient_flag:new(Decoded),
+                            [{Key, Parsed}];
+                        Bucket == segments ->
+                            Parsed = ldclient_segment:new(Decoded),
+                            [{Key, Parsed}]
+                    end
+            end;
+        {error, Reason} ->
+            error_logger:error_msg("Redis error looking up key ~p in bucket ~p: ~s", [Key, Bucket, format_error(Reason)]),
+            []
     end.
 
 %% @doc Upsert key value pairs in bucket
@@ -330,15 +340,28 @@ delete_key(true, Key, Bucket, Client, Prefix) ->
 bucket_name(Prefix, Bucket) ->
     lists:concat([Prefix, ":", Bucket]).
 
+%% @doc Format Redis error for logging
+%% @private
+%%
+%% @end
+-spec format_error(Reason :: no_connection | binary()) -> string().
+format_error(no_connection) -> "no connection to Redis";
+format_error(Reason) when is_binary(Reason) -> binary_to_list(Reason).
+
 -spec set_init(Client :: client_pid(), Prefix :: string()) -> ok.
 set_init(Client, Prefix) ->
     {ok, _} = eredis:q(Client, ["SET", lists:concat([Prefix, ":$inited"]), ""]),
     ok.
 
 -spec get_init(Client :: client_pid(), Prefix :: string()) -> boolean().
 get_init(Client, Prefix) ->
-    {ok, Value} = eredis:q(Client, ["GET", lists:concat([Prefix, ":$inited"])]),
-    case Value of
-        undefined -> false;
-        _ -> true
+    case eredis:q(Client, ["GET", lists:concat([Prefix, ":$inited"])]) of
+        {ok, Value} ->
+            case Value of
+                undefined -> false;
+                _ -> true
+            end;
+        {error, Reason} ->
+            error_logger:error_msg("Redis error getting init flag: ~s", [format_error(Reason)]),
+            false
     end.