Add support for supervisor one_for_all strategy

Adds support for handling restarts for the `one_for_all` strategy that was documented, but lacked
implementation. Makes necessary changes to ensure children are always restarted in the same order
they were originally started, and shutdown in reverse order with last child first, conforming to
OTP behavior.

Closes #1855

Signed-off-by: Winford <[email protected]>

Author: UncleGrumpy

CHANGELOG.md

@@ -92,6 +92,7 @@ instead `badarg`.
 - Fixed a bug where empty atom could not be created on some platforms, thus breaking receiving a message for a registered process from an OTP node.
 - Fix a memory leak in distribution when a BEAM node would monitor a process by name.
 - Fix `list_to_integer`, it was likely buggy with integers close to INT64_MAX
+- Added missing support for supervisor `one_for_all` strategy.
 
 ## [0.6.7] - Unreleased
 

libs/estdlib/src/supervisor.erl

@@ -83,18 +83,20 @@
     }.
 
 -record(child, {
-    pid = undefined,
+    pid = undefined :: pid() | undefined | {restarting, pid()} | {restarting, undefined},
     id :: any(),
     start :: {module(), atom(), [any()] | undefined},
     restart :: restart(),
     shutdown :: shutdown(),
     type :: child_type(),
     modules = [] :: [module()] | dynamic
 }).
--record(state, {restart_strategy :: strategy(), children = [] :: [#child{}]}).
+%% note: the list of children should always be kept in order, with last to start at the head.
+-record(state, {restart_strategy = one_for_one :: strategy(), children = [] :: [#child{}]}).
 
 start_link(Module, Args) ->
     gen_server:start_link(?MODULE, {Module, Args}, []).
+
 start_link(SupName, Module, Args) ->
     gen_server:start_link(SupName, ?MODULE, {Module, Args}, []).
 
@@ -172,7 +174,7 @@ child_spec_to_record(#{id := ChildId, start := MFA} = ChildMap) ->
 init_state([ChildSpec | T], State) ->
     Child = child_spec_to_record(ChildSpec),
     NewChildren = [Child | State#state.children],
-    init_state(T, #state{children = NewChildren});
+    init_state(T, State#state{children = NewChildren});
 init_state([], State) ->
     State#state{children = lists:reverse(State#state.children)}.
 
@@ -190,12 +192,16 @@ handle_call({start_child, ChildSpec}, _From, #state{children = Children} = State
     case lists:keyfind(ID, #child.id, State#state.children) of
         #child{pid = undefined} ->
             {reply, {error, already_present}, State};
+        #child{pid = {restarting, _Pid}} ->
+            {reply, {error, restarting}, State};
         #child{pid = Pid} ->
             {reply, {error, {already_started, Pid}}, State};
         false ->
             case try_start(Child) of
                 {ok, Pid, Result} ->
                     UpdatedChild = Child#child{pid = Pid},
+                    %% The last child to start should always be at the end of the child
+                    %% start list.
                     {reply, Result, State#state{children = [UpdatedChild | Children]}};
                 {error, _Reason} = ErrorT ->
                     {reply, ErrorT, State}
@@ -226,6 +232,8 @@ handle_call({restart_child, ID}, _From, #state{children = Children} = State) ->
                 {error, _Reason} = ErrorT ->
                     {reply, ErrorT, State}
             end;
+        #child{pid = {restarting, _}} ->
+            {reply, {error, restarting}, State};
         #child{} ->
             {reply, {error, running}, State};
         false ->
@@ -236,6 +244,8 @@ handle_call({delete_child, ID}, _From, #state{children = Children} = State) ->
         #child{pid = undefined} ->
             NewChildren = lists:keydelete(ID, #child.id, Children),
             {reply, ok, State#state{children = NewChildren}};
+        #child{pid = {restarting, _}} ->
+            {reply, {error, restarting}, State};
         #child{} ->
             {reply, {error, running}, State};
         false ->
@@ -254,12 +264,7 @@ handle_cast(_Msg, State) ->
     {noreply, State}.
 
 handle_info({'EXIT', Pid, Reason}, State) ->
-    case handle_child_exit(Pid, Reason, State) of
-        {ok, State1} ->
-            {noreply, State1};
-        {shutdown, State1} ->
-            {stop, shutdown, State1}
-    end;
+    handle_child_exit(Pid, Reason, State);
 handle_info({ensure_killed, Pid}, State) ->
     case lists:keyfind(Pid, #child.pid, State#state.children) of
         false ->
@@ -268,12 +273,31 @@ handle_info({ensure_killed, Pid}, State) ->
             exit(Pid, kill),
             {noreply, State}
     end;
+handle_info({restart_many_children, []}, State) ->
+    {noreply, State};
+handle_info(
+    {restart_many_children, [#child{pid = {restarting, _Pid0} = Pid} = Child | Children]}, State
+) ->
+    case try_start(Child) of
+        {ok, NewPid, _Result} ->
+            NewChild = Child#child{pid = NewPid},
+            NewChildren = lists:keyreplace(
+                Pid, #child.pid, State#state.children, NewChild
+            ),
+            {noreply, State#state{children = NewChildren},
+                {timeout, 0, {restart_many_children, Children}}};
+        {error, Reason} ->
+            handle_child_exit(Pid, Reason, State)
+    end;
+handle_info({restart_many_children, [#child{pid = undefined} = _Child | Children]}, State) ->
+    {noreply, State, {timeout, 0, {restart_many_children, Children}}};
 handle_info(_Msg, State) ->
     %TODO: log unexpected message
     {noreply, State}.
 
 %% @hidden
 terminate(_Reason, #state{children = Children} = State) ->
+    %% Shutdown children last to first.
     RemainingChildren = loop_terminate(Children, []),
     loop_wait_termination(RemainingChildren),
     {ok, State}.
@@ -284,34 +308,56 @@ terminate(_Reason, #state{children = Children} = State) ->
 handle_child_exit(Pid, Reason, State) ->
     case lists:keyfind(Pid, #child.pid, State#state.children) of
         false ->
-            {ok, State};
+            {noreply, State};
         #child{restart = {terminating, temporary, From}} ->
             gen_server:reply(From, ok),
             NewChildren = lists:keydelete(Pid, #child.pid, State#state.children),
-            {ok, State#state{children = NewChildren}};
+            {noreply, State#state{children = NewChildren}};
         #child{restart = {terminating, Restart, From}} = Child ->
             gen_server:reply(From, ok),
             NewChildren = lists:keyreplace(Pid, #child.pid, State#state.children, Child#child{
                 pid = undefined, restart = Restart
             }),
-            {ok, State#state{children = NewChildren}};
+            {noreply, State#state{children = NewChildren}};
         #child{} = Child ->
             case should_restart(Reason, Child#child.restart) of
                 true ->
-                    case try_start(Child) of
-                        {ok, NewPid, _Result} ->
-                            NewChild = Child#child{pid = NewPid},
-                            Children = lists:keyreplace(
-                                Pid, #child.pid, State#state.children, NewChild
-                            ),
-                            {ok, State#state{children = Children}}
-                    end;
+                    handle_restart_strategy(Child, State);
                 false ->
                     Children = lists:keydelete(Pid, #child.pid, State#state.children),
-                    {ok, State#state{children = Children}}
+                    {noreply, State#state{children = Children}}
             end
     end.
 
+handle_restart_strategy(
+    #child{id = Id} = Child, #state{restart_strategy = one_for_one} = State
+) ->
+    case try_start(Child) of
+        {ok, NewPid, _Result} ->
+            NewChild = Child#child{pid = NewPid},
+            Children = lists:keyreplace(
+                Id, #child.id, State#state.children, NewChild
+            ),
+            {noreply, State#state{children = Children}}
+    end;
+handle_restart_strategy(
+    #child{pid = Pid} = Child, #state{restart_strategy = one_for_all} = State
+) ->
+    Children =
+        case Pid of
+            {restarting, _} ->
+                State#state.children;
+            Pid when is_pid(Pid) ->
+                lists:keyreplace(Pid, #child.pid, State#state.children, Child#child{
+                    pid = {restarting, Pid}
+                })
+        end,
+    ok = terminate_one_for_all(Children),
+    {ok, NewChildren} = get_restart_children(Children),
+    %% NewChildren is startup order (first at head) and needs to be reversed to keep Children in correct order in #state{}
+    {noreply, State#state{children = lists:reverse(NewChildren)},
+        {timeout, 0, {restart_many_children, NewChildren}}}.
+
 should_restart(_Reason, permanent) ->
     true;
 should_restart(_Reason, temporary) ->
@@ -341,8 +387,7 @@ loop_wait_termination(RemainingChildren0) ->
             case lists:member(Pid, RemainingChildren0) of
                 true ->
                     exit(Pid, kill),
-                    RemainingChildren1 = lists:delete(Pid, RemainingChildren0),
-                    loop_wait_termination(RemainingChildren1);
+                    loop_wait_termination(RemainingChildren0);
                 false ->
                     loop_wait_termination(RemainingChildren0)
             end
@@ -367,6 +412,48 @@ try_start(#child{start = {M, F, Args}} = Record) ->
             {error, {{'EXIT', Error}, Record}}
     end.
 
+get_restart_children(Children) ->
+    get_restart_children(Children, []).
+
+get_restart_children([], NewChildren) ->
+    {ok, NewChildren};
+get_restart_children([Child | Children], NewChildren) ->
+    case Child of
+        #child{restart = {terminating, temporary, _From}} ->
+            get_restart_children(Children, NewChildren);
+        #child{restart = {terminating, _Restart, _From}} ->
+            get_restart_children(Children, [Child | NewChildren]);
+        #child{pid = undefined, restart = temporary} ->
+            get_restart_children(Children, NewChildren);
+        #child{pid = undefined} = Child ->
+            get_restart_children(Children, [Child | NewChildren]);
+        #child{pid = {restarting, _Pid}} = Child ->
+            get_restart_children(Children, [Child | NewChildren]);
+        #child{pid = Pid, restart = temporary} = Child when is_pid(Pid) ->
+            get_restart_children(Children, NewChildren);
+        #child{pid = Pid} = Child when is_pid(Pid) ->
+            get_restart_children(Children, [Child#child{pid = {restarting, Pid}} | NewChildren])
+    end.
+
+terminate_one_for_all(Children) ->
+    %% Always shut down last child first
+    do_terminate_one_for_all(Children, []).
+
+do_terminate_one_for_all([], StopPids) ->
+    ok = loop_wait_termination(StopPids),
+    %% After accumulation NewChildren are in correct order for restart.
+    ok;
+do_terminate_one_for_all([Child | Children], StopPids) ->
+    case Child of
+        #child{pid = Pid} = Child when is_pid(Pid) ->
+            do_terminate(Child),
+            do_terminate_one_for_all(Children, [Pid | StopPids]);
+        #child{pid = undefined} ->
+            do_terminate_one_for_all(Children, StopPids);
+        #child{pid = {restarting, _Pid}} = Child ->
+            do_terminate_one_for_all(Children, StopPids)
+    end.
+
 child_to_info(#child{id = Id, pid = Pid, type = Type, modules = Modules}) ->
     Child =
         case Pid of

tests/libs/estdlib/test_supervisor.erl

@@ -35,6 +35,7 @@ test() ->
     ok = test_terminate_timeout(),
     ok = test_which_children(),
     ok = test_count_children(),
+    ok = test_one_for_all(),
     ok.
 
 test_basic_supervisor() ->
@@ -292,7 +293,35 @@ init({test_supervisor_order, Parent}) ->
     ],
     {ok, {{one_for_one, 10000, 3600}, ChildSpecs}};
 init({test_no_child, _Parent}) ->
-    {ok, {#{strategy => one_for_one, intensity => 10000, period => 3600}, []}}.
+    {ok, {#{strategy => one_for_one, intensity => 10000, period => 3600}, []}};
+init({test_one_for_all, Parent}) ->
+    ChildSpecs = [
+        #{
+            id => ping_pong_1,
+            start => {ping_pong_server, start_link, [Parent]},
+            restart => permanent,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [ping_pong_server]
+        },
+        #{
+            id => ping_pong_2,
+            start => {ping_pong_server, start_link, [Parent]},
+            restart => transient,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [ping_pong_server]
+        },
+        #{
+            id => ready_0,
+            start => {notify_init_server, start_link, [{Parent, ready_0}]},
+            restart => temporary,
+            shutdown => brutal_kill,
+            type => worker,
+            modules => [notify_init_server]
+        }
+    ],
+    {ok, {#{strategy => one_for_all, intensity => 10000, period => 3600}, ChildSpecs}}.
 
 test_supervisor_order() ->
     {ok, SupPid} = supervisor:start_link(?MODULE, {test_supervisor_order, self()}),
@@ -346,3 +375,68 @@ test_which_children() ->
     unlink(SupPid),
     exit(SupPid, shutdown),
     ok.
+
+test_one_for_all() ->
+    {ok, SupPid} = supervisor:start_link({local, allforone}, ?MODULE, {test_one_for_all, self()}),
+    % Collect startup message from permanent ping_pong_server
+    Server_1 = get_and_test_server(),
+    % Collect startup message from transient ping_pong_server
+    Server_2 = get_and_test_server(),
+    % Collect startup message from temporary notify_init_server
+    ready_0 =
+        receive
+            Msg1 -> Msg1
+        after 1000 -> error({timeout, {start, ready_0}})
+        end,
+
+    [{specs, 3}, {active, 3}, {supervisors, 0}, {workers, 3}] = supervisor:count_children(SupPid),
+
+    %% Monitor transient Server_2 to make sure it is stopped, and restarted when
+    %% permanent Server_1 is shutdown.
+    MonRef = monitor(process, Server_2),
+    ok = gen_server:call(Server_1, {stop, test_crash}),
+    %% Server_2 should exit before the first child is restarted, but exit messages from
+    %% monitored processes may take some time to be received so we may get the message
+    %% from the first restarted child first.
+    First =
+        receive
+            {'DOWN', MonRef, process, Server_2, killed} ->
+                down;
+            {ping_pong_server_ready, Restart1} when is_pid(Restart1) ->
+                ready
+        after 1000 ->
+            error({timeout, restart_after_crash})
+        end,
+    ok =
+        case First of
+            down ->
+                receive
+                    {ping_pong_server_ready, Restart_1} when is_pid(Restart_1) -> ok
+                after 1000 ->
+                    error({timeout, restart_after_crash})
+                end;
+            ready ->
+                receive
+                    {'DOWN', MonRef, process, Server_2, killed} -> ok
+                after 1000 ->
+                    error({timeout, restart_after_crash})
+                end
+        end,
+
+    demonitor(MonRef),
+
+    % Collect startup message from restarted transient ping_pong_server child
+    _Restart_2 = get_and_test_server(),
+    % Make sure temporary notify_init_server is not restarted
+    no_start =
+        receive
+            ready_0 -> error({error, restarted_temporary})
+        after 1000 -> no_start
+        end,
+
+    % Ensure correct number of children
+    [{specs, 2}, {active, 2}, {supervisors, 0}, {workers, 2}] = supervisor:count_children(SupPid),
+
+    unlink(SupPid),
+    exit(SupPid, shutdown),
+    ok.