Use proplists for client options (Nordix#4)

Use an Erlang proplists for options when starting a client link

include/eredis.hrl

@@ -2,8 +2,10 @@
 
 -type reconnect_sleep() :: no_reconnect | integer().
 
--type option() :: {host, string()} | {port, integer()} | {database, string()} | {password, string()} | {reconnect_sleep, reconnect_sleep()}.
--type server_args() :: [option()].
+-type option() :: {database, string()} | {password, string()} | {reconnect_sleep, reconnect_sleep()} |
+                  {connect_timeout, integer()} | {socket_options, list()}.
+
+-type options() :: [option()].
 
 -type return_value() :: undefined | binary() | [binary() | nonempty_list()].
 

rebar.config

@@ -24,6 +24,11 @@
 {cover_excl_mods, [ basho_bench_driver_erldis
                   , basho_bench_driver_eredis]}.
 
+{dialyzer, [{warnings, [ error_handling
+                    %% , unmatched_returns
+                    %% , race_conditions
+                       ]}]}.
+
 {xref_checks, [ undefined_function_calls
               , locals_not_used
               , deprecated_function_calls

src/eredis.erl

@@ -13,68 +13,36 @@
 %% Specified in http://www.erlang.org/doc/man/gen_server.html#call-3
 -define(TIMEOUT, 5000).
 
--export([start_link/0, start_link/1, start_link/2, start_link/3, start_link/4,
-         start_link/5, start_link/6, start_link/7, stop/1, q/2, q/3, qp/2, qp/3, q_noreply/2,
-         q_async/2, q_async/3]).
+-export([start_link/0, start_link/2, start_link/3, stop/1]).
+-export([q/2, q/3, qp/2, qp/3, q_noreply/2, q_async/2, q_async/3]).
 
-%% Exported for testing
+%% Exported for eredis_sub_client and testing
 -export([create_multibulk/1]).
 
 %% Type of gen_server process id
--type client() :: pid() |
-                  atom() |
-                  {atom(), atom()} |
-                  {global, term()} |
-                  {via, atom(), term()}.
+-type client() :: pid().
 
 %%
 %% PUBLIC API
 %%
 
+-spec start_link() -> {ok, Pid::pid()} | {error, Reason::term()}.
 start_link() ->
-    start_link("127.0.0.1", 6379, 0, "").
+    start_link("127.0.0.1", 6379).
 
+-spec start_link(Host::list(), Port::integer()) -> {ok, Pid::pid()} | {error, Reason::term()}.
 start_link(Host, Port) ->
-    start_link(Host, Port, 0, "").
+    start_link(Host, Port, []).
 
-start_link(Host, Port, Database) ->
-    start_link(Host, Port, Database, "").
-
-start_link(Host, Port, Database, Password) ->
-    start_link(Host, Port, Database, Password, 100).
-
-start_link(Host, Port, Database, Password, ReconnectSleep) ->
-    start_link(Host, Port, Database, Password, ReconnectSleep, ?TIMEOUT).
-
-start_link(Host, Port, Database, Password, ReconnectSleep, ConnectTimeout) ->
-    start_link(Host, Port, Database, Password, ReconnectSleep, ConnectTimeout, []).
-
-start_link(Host, Port, Database, Password, ReconnectSleep, ConnectTimeout, SocketOptions)
+-spec start_link(Host::list(), Port::integer(), Options::options()) -> {ok, Pid::pid()} | {error, Reason::term()}.
+start_link(Host, Port, Options)
   when is_list(Host) orelse
-            (is_tuple(Host) andalso tuple_size(Host) =:= 2 andalso element(1, Host) =:= local),
+       (is_tuple(Host) andalso tuple_size(Host) =:= 2 andalso element(1, Host) =:= local),
        is_integer(Port),
-       is_integer(Database) orelse Database == undefined,
-       is_list(Password),
-       is_integer(ReconnectSleep) orelse ReconnectSleep =:= no_reconnect,
-       is_integer(ConnectTimeout),
-       is_list(SocketOptions) ->
-
-    eredis_client:start_link(Host, Port, Database, Password,
-                             ReconnectSleep, ConnectTimeout, SocketOptions).
-
-%% @doc: Callback for starting from poolboy
--spec start_link(server_args()) -> {ok, Pid::pid()} | {error, Reason::term()}.
-start_link(Args) ->
-    Host           = proplists:get_value(host, Args, "127.0.0.1"),
-    Port           = proplists:get_value(port, Args, 6379),
-    Database       = proplists:get_value(database, Args, 0),
-    Password       = proplists:get_value(password, Args, ""),
-    ReconnectSleep = proplists:get_value(reconnect_sleep, Args, 100),
-    ConnectTimeout = proplists:get_value(connect_timeout, Args, ?TIMEOUT),
-    SocketOptions  = proplists:get_value(socket_options, Args, []),
-
-    start_link(Host, Port, Database, Password, ReconnectSleep, ConnectTimeout, SocketOptions).
+       is_list(Options) ->
+    eredis_client:start_link(Host, Port, Options).
 
+-spec stop(Client::client()) -> ok.
 stop(Client) ->
     eredis_client:stop(Client).
 
@@ -87,6 +55,8 @@ stop(Client) ->
 q(Client, Command) ->
     call(Client, Command, ?TIMEOUT).
 
+-spec q(Client::client(), Command::[any()], Timeout::integer()) ->
+               {ok, return_value()} | {error, Reason::binary() | no_connection}.
 q(Client, Command, Timeout) ->
     call(Client, Command, Timeout).
 
@@ -101,6 +71,9 @@ q(Client, Command, Timeout) ->
 qp(Client, Pipeline) ->
     pipeline(Client, Pipeline, ?TIMEOUT).
 
+-spec qp(Client::client(), Pipeline::pipeline(), Timeout::integer()) ->
+                [{ok, return_value()} | {error, Reason::binary()}] |
+                {error, no_connection}.
 qp(Client, Pipeline, Timeout) ->
     pipeline(Client, Pipeline, Timeout).
 

src/eredis_client.erl

@@ -24,15 +24,19 @@
 -behaviour(gen_server).
 -include("eredis.hrl").
 
-%% API
--export([start_link/7, stop/1, select_database/2]).
+-define(CONNECT_TIMEOUT, 5000).
+-define(RECONNECT_SLEEP, 100).
 
--export([do_sync_command/2]).
+%% API
+-export([start_link/3, stop/1, select_database/2]).
 
 %% gen_server callbacks
 -export([init/1, handle_call/3, handle_cast/2, handle_info/2,
          terminate/2, code_change/3]).
 
+%% Used by eredis_sub_client.erl
+-export([do_sync_command/2]).
+
 -record(state, {
                 host :: string() | {local, string()} | undefined,
                 port :: integer() | undefined,
@@ -53,15 +57,10 @@
 
 -spec start_link(Host::list(),
                  Port::integer(),
-                 Database::integer() | undefined,
-                 Password::string(),
-                 ReconnectSleep::reconnect_sleep(),
-                 ConnectTimeout::integer() | undefined,
-                 SocketOptions::list()) ->
+                 Options::options()) ->
           {ok, Pid::pid()} | {error, Reason::term()}.
-start_link(Host, Port, Database, Password, ReconnectSleep, ConnectTimeout, SocketOptions) ->
-    gen_server:start_link(?MODULE, [Host, Port, Database, Password,
-                                    ReconnectSleep, ConnectTimeout, SocketOptions], []).
+start_link(Host, Port, Options) ->
+    gen_server:start_link(?MODULE, [Host, Port, Options], []).
 
 
 stop(Pid) ->
@@ -71,7 +70,13 @@ stop(Pid) ->
 %% gen_server callbacks
 %%====================================================================
 
-init([Host, Port, Database, Password, ReconnectSleep, ConnectTimeout, SocketOptions]) ->
+init([Host, Port, Options]) ->
+    Database       = proplists:get_value(database, Options, 0),
+    Password       = proplists:get_value(password, Options, ""),
+    ReconnectSleep = proplists:get_value(reconnect_sleep, Options, ?RECONNECT_SLEEP),
+    ConnectTimeout = proplists:get_value(connect_timeout, Options, ?CONNECT_TIMEOUT),
+    SocketOptions  = proplists:get_value(socket_options, Options, []),
+
     State = #state{host = Host,
                    port = Port,
                    database = read_database(Database),
@@ -80,6 +85,7 @@ init([Host, Port, Database, Password, ReconnectSleep, ConnectTimeout, SocketOpti
                    connect_timeout = ConnectTimeout,
                    socket_options = SocketOptions,
 
+                   socket = undefined,
                    parser_state = eredis_parser:init(),
                    queue = queue:new()},
 

src/eredis_sub.erl

@@ -37,7 +37,7 @@ start_link(Host, Port, Password, ReconnectSleep,
                                  MaxQueueSize, QueueBehaviour).
 
 %% @doc: Callback for starting from poolboy
--spec start_link(server_args()) -> {ok, Pid::pid()} | {error, Reason::term()}.
+-spec start_link(options()) -> {ok, Pid::pid()} | {error, Reason::term()}.
 start_link(Args) ->
     Host           = proplists:get_value(host, Args, "127.0.0.1"),
     Port           = proplists:get_value(port, Args, 6379),

test/eredis_tests.erl

@@ -6,16 +6,19 @@
 -import(eredis, [create_multibulk/1]).
 
 connect_test() ->
+    ?assertMatch({ok, _}, eredis:start_link()),
     ?assertMatch({ok, _}, eredis:start_link("127.0.0.1", 6379)),
-    ?assertMatch({ok, _}, eredis:start_link("localhost", 6379)).
-
-connect_socket_options_test() ->
-    ?assertMatch({ok, _}, eredis:start_link([{socket_options, [{keepalive, true}]}])),
-    ?assertMatch({ok, _}, eredis:start_link("localhost", 6379, 0, "", 100, 5000, [{keepalive, true}])).
+    ?assertMatch({ok, _}, eredis:start_link("localhost", 6379)),
+    ?assertMatch({ok, _}, eredis:start_link("localhost", 6379, [{database, 0},
+                                                                {password, ""},
+                                                                {reconnect_sleep, 100},
+                                                                {connect_timeout, 5000},
+                                                                {socket_options, [{keepalive, true}]}
+                                                               ])).
 
 connect_local_test() ->
     process_flag(trap_exit, true),
-    eredis:start_link({local, "/var/run/redis.sock"}, 0, 0, "", no_reconnect),
+    eredis:start_link({local, "/var/run/redis.sock"}, 0, [{reconnect_sleep, no_reconnect}]),
     IsDead = receive {'EXIT', _Pid, {connection_error, enoent}} -> died
              after 400 -> still_alive end,
     process_flag(trap_exit, false),
@@ -160,18 +163,6 @@ q_async_test() ->
             ?assertMatch({ok, _}, eredis:q(C, ["DEL", foo]))
     end.
 
-c() ->
-    Res = eredis:start_link(),
-    ?assertMatch({ok, _}, Res),
-    {ok, C} = Res,
-    C.
-
-c_no_reconnect() ->
-    Res = eredis:start_link("127.0.0.1", 6379, 0, "", no_reconnect),
-    ?assertMatch({ok, _}, Res),
-    {ok, C} = Res,
-    C.
-
 multibulk_test_() ->
     [?_assertEqual(<<"*3\r\n$3\r\nSET\r\n$3\r\nfoo\r\n$3\r\nbar\r\n">>,
                    list_to_binary(create_multibulk(["SET", "foo", "bar"]))),
@@ -186,14 +177,17 @@ multibulk_test_() ->
     ].
 
 undefined_database_test() ->
-    ?assertMatch({ok, _}, eredis:start_link("localhost", 6379, undefined)).
+    ?assertMatch({ok, _}, eredis:start_link("localhost", 6379, [{database, undefined}])).
 
 select_logical_database_test() ->
-    ?assertMatch({ok, _}, eredis:start_link("localhost", 6379, 2)).
+    ?assertMatch({ok, _}, eredis:start_link("localhost", 6379, [{database, 2},
+                                                                {reconnect_sleep, no_reconnect}])).
 
 authentication_error_test() ->
     process_flag(trap_exit, true),
-    Res = eredis:start_link("127.0.0.1", 6379, 4, "password", no_reconnect),
+    Res = eredis:start_link("127.0.0.1", 6379, [{database, 4},
+                                                {password, "password"},
+                                                {reconnect_sleep, no_reconnect}]),
     ?assertMatch({error, {authentication_error, _}}, Res),
     IsDead = receive {'EXIT', _, _} -> died
              after 1000 -> still_alive end,
@@ -202,7 +196,8 @@ authentication_error_test() ->
 
 connection_failure_during_start_no_reconnect_test() ->
     process_flag(trap_exit, true),
-    Res = eredis:start_link("localhost", 6378, 0, "", no_reconnect),
+    Res = eredis:start_link("localhost", 6378, [{reconnect_sleep, no_reconnect},
+                                                {connect_timeout, 1000}]),
     ?assertMatch({error, _}, Res),
     IsDead = receive {'EXIT', _, _} -> died
              after 1000 -> still_alive end,
@@ -211,7 +206,7 @@ connection_failure_during_start_no_reconnect_test() ->
 
 connection_failure_during_start_reconnect_test() ->
     process_flag(trap_exit, true),
-    Res = eredis:start_link("localhost", 6378, 0, "", 100),
+    Res = eredis:start_link("localhost", 6378, [{reconnect_sleep, 100}]),
     ?assertMatch({ok, _}, Res),
     {ok, ClientPid} = Res,
     IsDead = receive {'EXIT', ClientPid, _} -> died
@@ -237,6 +232,10 @@ tcp_closed_no_reconnect_test() ->
     C = c_no_reconnect(),
     tcp_closed_rig(C).
 
+%%
+%% Helpers
+%%
+
 tcp_closed_rig(C) ->
     %% fire async requests to add to redis client queue and then trick
     %% the client into thinking the connection to redis has been
@@ -276,3 +275,15 @@ gather_remote_queries([Pid | Rest], Acc) ->
         10000 ->
             error({gather_remote_queries, timeout})
     end.
+
+c() ->
+    Res = eredis:start_link(),
+    ?assertMatch({ok, _}, Res),
+    {ok, C} = Res,
+    C.
+
+c_no_reconnect() ->
+    Res = eredis:start_link("127.0.0.1", 6379, [{reconnect_sleep, no_reconnect}]),
+    ?assertMatch({ok, _}, Res),
+    {ok, C} = Res,
+    C.