fix(supervision tree): supervisors should be permanent; allow customizing worker shutdown and restart

.github/workflows/erlang.yml

@@ -18,6 +18,7 @@ jobs:
           - otp_release: 25
           - otp_release: 26
           - otp_release: 27
+          - otp_release: 28
 
     steps:
     - uses: actions/checkout@v2

src/ecpool.erl

@@ -66,6 +66,10 @@
     | {auto_reconnect, false | pos_integer()}
     | {on_reconnect, conn_callback()}
     | {on_disconnect, conn_callback()}
+      %% default: transient
+    | {worker_restart, transient | permanent | temporary}
+      %% default: 2_000
+    | {worker_shutdown, brutal_kill | timeout()}
     | tuple().
 -type get_client_ret() :: pid() | false | no_such_pool.
 -type start_error() :: no_worker_sup

src/ecpool_pool_sup.erl

@@ -36,15 +36,15 @@ init([PoolName, Mod, Opts]) ->
     Pool = #{
         id => pool,
         start => {ecpool_pool, start_link, [PoolName, Opts]},
-        restart => transient,
+        restart => permanent,
         shutdown => 16#ffff,
         type => worker,
         modules => [ecpool_pool]
     },
     WorkerSup = #{
         id => worker_sup,
         start => {ecpool_worker_sup,start_link, [PoolName, Mod, Opts]},
-        restart => transient,
+        restart => permanent,
         shutdown => infinity,
         type => supervisor,
         modules => [ecpool_worker_sup]

src/ecpool_sup.erl

@@ -96,7 +96,7 @@ pool_spec(Pool, Mod, Opts) ->
       start => {ecpool_pool_sup,
                 start_link,
                 [Pool, Mod, Opts]},
-      restart => transient,
+      restart => permanent,
       shutdown => infinity,
       type => supervisor,
       modules => [ecpool_pool_sup]}.

src/ecpool_worker.erl

@@ -173,7 +173,7 @@ handle_continue(connect, State) ->
 handle_call(take_start_result, _From, State) ->
     {reply, ?take_start_result(), State};
 handle_call(is_connected, _From, State = #state{client = Client}) when is_pid(Client) ->
-    IsAlive = Client =/= undefined andalso is_process_alive(Client),
+    IsAlive = is_process_alive(Client),
     {reply, IsAlive, State};
 handle_call(is_connected, _From, State = #state{client = Client}) ->
     {reply, Client =/= undefined, State};

src/ecpool_worker_sup.erl

@@ -35,13 +35,15 @@ init([Pool, Mod, Opts]) ->
         intensity => 10 + PoolSize,
         period => 60
     },
+    Restart = proplists:get_value(worker_restart, Opts, transient),
+    Shutdown = proplists:get_value(worker_shutdown, Opts, 2_000),
     WorkerSpec = fun(Id) ->
                      #{id => {worker, Id},
                        start => {ecpool_worker,
                                  start_link,
                                  [Pool, Id, Mod, Opts]},
-                       restart => transient,
-                       shutdown => 2_000,
+                       restart => Restart,
+                       shutdown => Shutdown,
                        type => worker,
                        modules => [ecpool_worker, Mod]}
                  end,

test/ecpool_SUITE.erl

@@ -82,7 +82,6 @@ groups() ->
        t_multiprocess_client,
        t_multiprocess_client_not_restart,
        t_pick_and_do_fun,
-       t_check_pool_integrity,
        t_big_pool_dies_and_recovers
       ]}].
 
@@ -353,29 +352,6 @@ t_pick_and_do_fun(_Config) ->
     ?assertEqual(4, ecpool:pick_and_do({Pool, <<"abc">>}, Action, no_handover)),
     ecpool:stop_sup_pool(Pool).
 
-%% Smoke tests for `ecpool:check_pool_integrity`, which should report an error when worker
-%% supervisor is down.
-t_check_pool_integrity(_TCConfig) ->
-    Pool = ?FUNCTION_NAME,
-    Opts1 = [ {pool_size, 15}
-            , {pool_type, hash}
-            , {auto_reconnect, false}
-            ],
-    {ok, _} = ecpool:start_sup_pool(Pool, test_client, Opts1),
-    ?assertEqual(ok, ecpool:check_pool_integrity(Pool)),
-    ok = ecpool:stop_sup_pool(Pool),
-    Opts2 = [ {crash_after, 1}
-            , {auto_reconnect, true}
-            | Opts1
-            ],
-    {ok, _} = ecpool:start_sup_pool(Pool, test_client, Opts2),
-    %% Give it some time to reach maximum restart intensity
-    ct:sleep(100),
-    ?assertEqual({error, {processes_down, [worker_sup]}}, ecpool:check_pool_integrity(Pool)),
-    ok = ecpool:stop_sup_pool(Pool),
-    ?assertEqual({error, not_found}, ecpool:check_pool_integrity(Pool)),
-    ok.
-
 %% Previously, we had a fixed restart intensity for the worker supervisor, meaning that if
 %% a large pool dies once, it brings down the supervisor.  This checks that we have an
 %% intensity proportional to the pool size, so the whole pool may restart at once without