From 8eb8c17d49c98a1c0c33d4a9e0630a7b5a2e2535 Mon Sep 17 00:00:00 2001 From: Matthew Sackman Date: Mon, 9 Aug 2010 12:32:27 +0100 Subject: Introduce intrinsic restart type --- src/supervisor2.erl | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index fb4c9b02..eb2c6bde 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -31,6 +31,14 @@ %% the MaxT and MaxR parameters to permit the child to be %% restarted. This may require waiting for longer than Delay. %% +%% 4) Added an 'intrinsic' restart type. This type means that the +%% child should never be restarted (same as temporary) but whenever +%% such a child exits, it will cause the entire supervisor to exit +%% (i.e. the child's existence is intrinsic to the supervisor's +%% existence). Because such children are never restarted, the +%% supervisor's restart strategy, MaxT and MaxR have no bearing on +%% such children. +%% %% All modifications are (C) 2010 Rabbit Technologies Ltd. %% %% %CopyrightBegin% @@ -521,6 +529,12 @@ restart_child(Pid, Reason, State) -> {ok, State} end. +do_restart(intrinsic, Reason, Child, State = #state{name = Name}) -> + case Reason of + normal -> ok; + _ -> report_error(child_terminated, Reason, Child, Name) + end, + {shutdown, remove_child(Child, State)}; do_restart({RestartType, Delay}, Reason, Child, State) -> case restart1(Child, State) of {ok, NState} -> @@ -834,7 +848,7 @@ supname(N,_) -> N. %%% where Name is an atom %%% Func is {Mod, Fun, Args} == {atom, atom, list} %%% RestartType is permanent | temporary | transient | -%%% {permanent, Delay} | +%%% intrinsic | {permanent, Delay} | %%% {transient, Delay} where Delay >= 0 %%% Shutdown = integer() | infinity | brutal_kill %%% ChildType = supervisor | worker @@ -881,6 +895,7 @@ validFunc({M, F, A}) when is_atom(M), is_list(A) -> true; validFunc(Func) -> throw({invalid_mfa, Func}). +validRestartType(intrinsic) -> true; validRestartType(permanent) -> true; validRestartType(temporary) -> true; validRestartType(transient) -> true; -- cgit v1.2.1 From 7bd896d397bf7918f9757593f44c21711b13d867 Mon Sep 17 00:00:00 2001 From: Matthew Sackman Date: Mon, 9 Aug 2010 13:38:27 +0100 Subject: Convert rabbit_sup to supervisor2 using intrinsic --- src/rabbit_sup.erl | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/rabbit_sup.erl b/src/rabbit_sup.erl index 97613d17..bf29d542 100644 --- a/src/rabbit_sup.erl +++ b/src/rabbit_sup.erl @@ -31,7 +31,7 @@ -module(rabbit_sup). --behaviour(supervisor). +-behaviour(supervisor2). -export([start_link/0, start_child/1, start_child/2, start_child/3, start_restartable_child/1, start_restartable_child/2, stop_child/1]). @@ -43,7 +43,7 @@ -define(SERVER, ?MODULE). start_link() -> - supervisor:start_link({local, ?SERVER}, ?MODULE, []). + supervisor2:start_link({local, ?SERVER}, ?MODULE, []). start_child(Mod) -> start_child(Mod, []). @@ -52,9 +52,9 @@ start_child(Mod, Args) -> start_child(Mod, Mod, Args). start_child(ChildId, Mod, Args) -> - {ok, _} = supervisor:start_child(?SERVER, - {ChildId, {Mod, start_link, Args}, - transient, ?MAX_WAIT, worker, [Mod]}), + {ok, _} = supervisor2:start_child(?SERVER, + {ChildId, {Mod, start_link, Args}, + intrinsic, ?MAX_WAIT, worker, [Mod]}), ok. start_restartable_child(Mod) -> @@ -62,16 +62,16 @@ start_restartable_child(Mod) -> start_restartable_child(Mod, Args) -> Name = list_to_atom(atom_to_list(Mod) ++ "_sup"), - {ok, _} = supervisor:start_child( + {ok, _} = supervisor2:start_child( ?SERVER, {Name, {rabbit_restartable_sup, start_link, [Name, {Mod, start_link, Args}]}, - transient, infinity, supervisor, [rabbit_restartable_sup]}), + intrinsic, infinity, supervisor, [rabbit_restartable_sup]}), ok. stop_child(ChildId) -> - case supervisor:terminate_child(?SERVER, ChildId) of - ok -> supervisor:delete_child(?SERVER, ChildId); + case supervisor2:terminate_child(?SERVER, ChildId) of + ok -> supervisor2:delete_child(?SERVER, ChildId); E -> E end. -- cgit v1.2.1 From 327844e52acfc7dab360f30070b81793bc065491 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Mon, 9 Aug 2010 15:34:22 +0100 Subject: cosmetic --- src/supervisor2.erl | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index eb2c6bde..902ad8d0 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -529,12 +529,6 @@ restart_child(Pid, Reason, State) -> {ok, State} end. -do_restart(intrinsic, Reason, Child, State = #state{name = Name}) -> - case Reason of - normal -> ok; - _ -> report_error(child_terminated, Reason, Child, Name) - end, - {shutdown, remove_child(Child, State)}; do_restart({RestartType, Delay}, Reason, Child, State) -> case restart1(Child, State) of {ok, NState} -> @@ -545,6 +539,11 @@ do_restart({RestartType, Delay}, Reason, Child, State) -> [self(), {{RestartType, Delay}, Reason, Child}]), {ok, NState} end; +do_restart(intrinsic, normal, Child, State) -> + {shutdown, remove_child(Child, State)}; +do_restart(intrinsic, Reason, Child, State) -> + report_error(child_terminated, Reason, Child, State#state.name), + {shutdown, remove_child(Child, State)}; do_restart(permanent, Reason, Child, State) -> report_error(child_terminated, Reason, Child, State#state.name), restart(Child, State); @@ -847,8 +846,8 @@ supname(N,_) -> N. %%% {Name, Func, RestartType, Shutdown, ChildType, Modules} %%% where Name is an atom %%% Func is {Mod, Fun, Args} == {atom, atom, list} -%%% RestartType is permanent | temporary | transient | -%%% intrinsic | {permanent, Delay} | +%%% RestartType is intrinsic | permanent | temporary | +%%% transient | {permanent, Delay} | %%% {transient, Delay} where Delay >= 0 %%% Shutdown = integer() | infinity | brutal_kill %%% ChildType = supervisor | worker -- cgit v1.2.1 From 97b2cedc18207b655a0db0b1546617634c272ce5 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Mon, 9 Aug 2010 16:01:58 +0100 Subject: use state_del_child instead of remove_child remove_child removes child *specs* which is not what we want here --- src/supervisor2.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index 902ad8d0..3875997e 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -540,10 +540,10 @@ do_restart({RestartType, Delay}, Reason, Child, State) -> {ok, NState} end; do_restart(intrinsic, normal, Child, State) -> - {shutdown, remove_child(Child, State)}; + {shutdown, state_del_child(Child, State)}; do_restart(intrinsic, Reason, Child, State) -> report_error(child_terminated, Reason, Child, State#state.name), - {shutdown, remove_child(Child, State)}; + {shutdown, state_del_child(Child, State)}; do_restart(permanent, Reason, Child, State) -> report_error(child_terminated, Reason, Child, State#state.name), restart(Child, State); -- cgit v1.2.1 From 8f4756b806d9029c297c6cdc3ea111f5a97e9085 Mon Sep 17 00:00:00 2001 From: Matthew Sackman Date: Mon, 9 Aug 2010 16:35:39 +0100 Subject: Reverting rabbit_sup to not use intrinsic --- src/rabbit_sup.erl | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/rabbit_sup.erl b/src/rabbit_sup.erl index bf29d542..97613d17 100644 --- a/src/rabbit_sup.erl +++ b/src/rabbit_sup.erl @@ -31,7 +31,7 @@ -module(rabbit_sup). --behaviour(supervisor2). +-behaviour(supervisor). -export([start_link/0, start_child/1, start_child/2, start_child/3, start_restartable_child/1, start_restartable_child/2, stop_child/1]). @@ -43,7 +43,7 @@ -define(SERVER, ?MODULE). start_link() -> - supervisor2:start_link({local, ?SERVER}, ?MODULE, []). + supervisor:start_link({local, ?SERVER}, ?MODULE, []). start_child(Mod) -> start_child(Mod, []). @@ -52,9 +52,9 @@ start_child(Mod, Args) -> start_child(Mod, Mod, Args). start_child(ChildId, Mod, Args) -> - {ok, _} = supervisor2:start_child(?SERVER, - {ChildId, {Mod, start_link, Args}, - intrinsic, ?MAX_WAIT, worker, [Mod]}), + {ok, _} = supervisor:start_child(?SERVER, + {ChildId, {Mod, start_link, Args}, + transient, ?MAX_WAIT, worker, [Mod]}), ok. start_restartable_child(Mod) -> @@ -62,16 +62,16 @@ start_restartable_child(Mod) -> start_restartable_child(Mod, Args) -> Name = list_to_atom(atom_to_list(Mod) ++ "_sup"), - {ok, _} = supervisor2:start_child( + {ok, _} = supervisor:start_child( ?SERVER, {Name, {rabbit_restartable_sup, start_link, [Name, {Mod, start_link, Args}]}, - intrinsic, infinity, supervisor, [rabbit_restartable_sup]}), + transient, infinity, supervisor, [rabbit_restartable_sup]}), ok. stop_child(ChildId) -> - case supervisor2:terminate_child(?SERVER, ChildId) of - ok -> supervisor2:delete_child(?SERVER, ChildId); + case supervisor:terminate_child(?SERVER, ChildId) of + ok -> supervisor:delete_child(?SERVER, ChildId); E -> E end. -- cgit v1.2.1 From 645aea572e913b9d122cfb99370a5d5fdf69f5a5 Mon Sep 17 00:00:00 2001 From: Matthew Sackman Date: Mon, 9 Aug 2010 16:36:44 +0100 Subject: Modify behaviour of intrinsic to be more like transient, except in the case of normal child exit --- src/supervisor2.erl | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index 3875997e..8d0f5f4b 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -31,13 +31,10 @@ %% the MaxT and MaxR parameters to permit the child to be %% restarted. This may require waiting for longer than Delay. %% -%% 4) Added an 'intrinsic' restart type. This type means that the -%% child should never be restarted (same as temporary) but whenever -%% such a child exits, it will cause the entire supervisor to exit -%% (i.e. the child's existence is intrinsic to the supervisor's -%% existence). Because such children are never restarted, the -%% supervisor's restart strategy, MaxT and MaxR have no bearing on -%% such children. +%% 4) Added an 'intrinsic' restart type. Like the transient type, this +%% type means the child should only be restarted if the child exits +%% abnormally. Unlike the transient type, if the child exits +%% normally, the supervisor itself also exits normally. %% %% All modifications are (C) 2010 Rabbit Technologies Ltd. %% @@ -541,9 +538,6 @@ do_restart({RestartType, Delay}, Reason, Child, State) -> end; do_restart(intrinsic, normal, Child, State) -> {shutdown, state_del_child(Child, State)}; -do_restart(intrinsic, Reason, Child, State) -> - report_error(child_terminated, Reason, Child, State#state.name), - {shutdown, state_del_child(Child, State)}; do_restart(permanent, Reason, Child, State) -> report_error(child_terminated, Reason, Child, State#state.name), restart(Child, State); @@ -553,7 +547,8 @@ do_restart(_, normal, Child, State) -> do_restart(_, shutdown, Child, State) -> NState = state_del_child(Child, State), {ok, NState}; -do_restart(transient, Reason, Child, State) -> +do_restart(Type, Reason, Child, State) when Type =:= transient orelse + Type =:= intrinsic -> report_error(child_terminated, Reason, Child, State#state.name), restart(Child, State); do_restart(temporary, Reason, Child, State) -> -- cgit v1.2.1 From 6587526953a04fdd5eb012650c46d7f169238baa Mon Sep 17 00:00:00 2001 From: Matthew Sackman Date: Tue, 10 Aug 2010 12:53:40 +0100 Subject: Prevent supervisor2 from getting upset if a child has already vanished normally whilst the supervisor is trying to stop it --- src/supervisor2.erl | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index 8d0f5f4b..b1b3760b 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -676,10 +676,13 @@ shutdown(Pid, brutal_kill) -> ok -> exit(Pid, kill), receive - {'DOWN', _MRef, process, Pid, killed} -> - ok; {'DOWN', _MRef, process, Pid, OtherReason} -> - {error, OtherReason} + case OtherReason of + killed -> ok; + normal -> ok; + noproc -> ok; + _ -> {error, OtherReason} + end end; {error, Reason} -> {error, Reason} @@ -691,10 +694,13 @@ shutdown(Pid, Time) -> ok -> exit(Pid, shutdown), %% Try to shutdown gracefully receive - {'DOWN', _MRef, process, Pid, shutdown} -> - ok; {'DOWN', _MRef, process, Pid, OtherReason} -> - {error, OtherReason} + case OtherReason of + shutdown -> ok; + normal -> ok; + noproc -> ok; + _ -> {error, OtherReason} + end after Time -> exit(Pid, kill), %% Force termination. receive @@ -720,10 +726,13 @@ monitor_child(Pid) -> %% If the child dies before the unlik we must empty %% the mail-box of the 'EXIT'-message and the 'DOWN'-message. {'EXIT', Pid, Reason} -> - receive - {'DOWN', _, process, Pid, _} -> - {error, Reason} - end + case Reason of + normal -> ok; + _ -> receive + {'DOWN', _, process, Pid, _} -> + {error, Reason} + end + end after 0 -> %% If a naughty child did unlink and the child dies before %% monitor the result will be that shutdown/2 receives a -- cgit v1.2.1 From b65494db2ac7e5127d0c5ab861525a556ac02e4a Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 12 Aug 2010 05:19:28 +0100 Subject: cosmetic and minor refactoring --- src/supervisor2.erl | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index b1b3760b..75c4bc11 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -536,11 +536,11 @@ do_restart({RestartType, Delay}, Reason, Child, State) -> [self(), {{RestartType, Delay}, Reason, Child}]), {ok, NState} end; -do_restart(intrinsic, normal, Child, State) -> - {shutdown, state_del_child(Child, State)}; do_restart(permanent, Reason, Child, State) -> report_error(child_terminated, Reason, Child, State#state.name), restart(Child, State); +do_restart(intrinsic, normal, Child, State) -> + {shutdown, state_del_child(Child, State)}; do_restart(_, normal, Child, State) -> NState = state_del_child(Child, State), {ok, NState}; @@ -676,13 +676,8 @@ shutdown(Pid, brutal_kill) -> ok -> exit(Pid, kill), receive - {'DOWN', _MRef, process, Pid, OtherReason} -> - case OtherReason of - killed -> ok; - normal -> ok; - noproc -> ok; - _ -> {error, OtherReason} - end + {'DOWN', _MRef, process, Pid, Reason} -> + check_shutdown_reason(killed, Reason) end; {error, Reason} -> {error, Reason} @@ -694,13 +689,8 @@ shutdown(Pid, Time) -> ok -> exit(Pid, shutdown), %% Try to shutdown gracefully receive - {'DOWN', _MRef, process, Pid, OtherReason} -> - case OtherReason of - shutdown -> ok; - normal -> ok; - noproc -> ok; - _ -> {error, OtherReason} - end + {'DOWN', _MRef, process, Pid, Reason} -> + check_shutdown_reason(shutdown, Reason) after Time -> exit(Pid, kill), %% Force termination. receive @@ -742,8 +732,12 @@ monitor_child(Pid) -> %% that will be handled in shutdown/2. ok end. - - + +check_shutdown_reason(Reason, Reason) -> ok; +check_shutdown_reason(_ , normal) -> ok; +check_shutdown_reason(_ , noproc) -> ok; +check_shutdown_reason(Reason, _) -> {error, Reason}. + %%----------------------------------------------------------------- %% Child/State manipulating functions. %%----------------------------------------------------------------- @@ -850,8 +844,8 @@ supname(N,_) -> N. %%% {Name, Func, RestartType, Shutdown, ChildType, Modules} %%% where Name is an atom %%% Func is {Mod, Fun, Args} == {atom, atom, list} -%%% RestartType is intrinsic | permanent | temporary | -%%% transient | {permanent, Delay} | +%%% RestartType is permanent | temporary | transient | +%%% intrinsic | {permanent, Delay} | %%% {transient, Delay} where Delay >= 0 %%% Shutdown = integer() | infinity | brutal_kill %%% ChildType = supervisor | worker @@ -898,10 +892,10 @@ validFunc({M, F, A}) when is_atom(M), is_list(A) -> true; validFunc(Func) -> throw({invalid_mfa, Func}). -validRestartType(intrinsic) -> true; validRestartType(permanent) -> true; validRestartType(temporary) -> true; validRestartType(transient) -> true; +validRestartType(intrinsic) -> true; validRestartType({permanent, Delay}) -> validDelay(Delay); validRestartType({transient, Delay}) -> validDelay(Delay); validRestartType(RestartType) -> throw({invalid_restart_type, -- cgit v1.2.1 From f1282da0f8e33c7e9305b67cdcf2e9ba03d5c2e3 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 12 Aug 2010 05:23:40 +0100 Subject: we must always clear both the 'EXIT' and 'DOWN' message --- src/supervisor2.erl | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index 75c4bc11..edd43ec0 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -715,13 +715,13 @@ monitor_child(Pid) -> receive %% If the child dies before the unlik we must empty %% the mail-box of the 'EXIT'-message and the 'DOWN'-message. - {'EXIT', Pid, Reason} -> - case Reason of - normal -> ok; - _ -> receive - {'DOWN', _, process, Pid, _} -> - {error, Reason} - end + {'EXIT', Pid, Reason} -> + receive + {'DOWN', _, process, Pid, _} -> + case Reason of + normal -> ok; + _ -> {error, Reason} + end end after 0 -> %% If a naughty child did unlink and the child dies before -- cgit v1.2.1 From d537bc6dfee5fd60b17a491bc46e7c9c78ce9bfb Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 12 Aug 2010 05:39:02 +0100 Subject: never ignore 'noproc' during shutdown We have no idea why the child died. Furthermore, we will only ever get this when a child has unlinked itself from the supervisor, which is naughty and shouldn't go unnoticed. --- src/supervisor2.erl | 1 - 1 file changed, 1 deletion(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index edd43ec0..dc55dbb6 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -735,7 +735,6 @@ monitor_child(Pid) -> check_shutdown_reason(Reason, Reason) -> ok; check_shutdown_reason(_ , normal) -> ok; -check_shutdown_reason(_ , noproc) -> ok; check_shutdown_reason(Reason, _) -> {error, Reason}. %%----------------------------------------------------------------- -- cgit v1.2.1 From 5a2cdf2ad6936a24a356497d10d0b0eb2a8f8801 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Thu, 12 Aug 2010 06:16:57 +0100 Subject: do not ignore 'normal' permanent child terminations during shutdown If such a termination is worth reporting while the supervisor is running, it sure is still worth reporting when it's shutting down. This involves reverting all the changes related to ignoring normal termination during shutdown and instead adding some logic a bit higher up the call chain. --- src/supervisor2.erl | 56 +++++++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/src/supervisor2.erl b/src/supervisor2.erl index dc55dbb6..4d2955d9 100644 --- a/src/supervisor2.erl +++ b/src/supervisor2.erl @@ -649,14 +649,22 @@ terminate_simple_children(Child, Dynamics, SupName) -> ok. do_terminate(Child, SupName) when Child#child.pid =/= undefined -> - case shutdown(Child#child.pid, - Child#child.shutdown) of - ok -> - Child#child{pid = undefined}; - {error, OtherReason} -> - report_error(shutdown_error, OtherReason, Child, SupName), - Child#child{pid = undefined} - end; + ReportError = fun (Reason) -> + report_error(shutdown_error, Reason, Child, SupName) + end, + case shutdown(Child#child.pid, Child#child.shutdown) of + ok -> + ok; + {error, normal} -> + case Child#child.restart_type of + permanent -> ReportError(normal); + {permanent, _Delay} -> ReportError(normal); + _ -> ok + end; + {error, OtherReason} -> + ReportError(OtherReason) + end, + Child#child{pid = undefined}; do_terminate(Child, _SupName) -> Child. @@ -676,8 +684,10 @@ shutdown(Pid, brutal_kill) -> ok -> exit(Pid, kill), receive - {'DOWN', _MRef, process, Pid, Reason} -> - check_shutdown_reason(killed, Reason) + {'DOWN', _MRef, process, Pid, killed} -> + ok; + {'DOWN', _MRef, process, Pid, OtherReason} -> + {error, OtherReason} end; {error, Reason} -> {error, Reason} @@ -689,8 +699,10 @@ shutdown(Pid, Time) -> ok -> exit(Pid, shutdown), %% Try to shutdown gracefully receive - {'DOWN', _MRef, process, Pid, Reason} -> - check_shutdown_reason(shutdown, Reason) + {'DOWN', _MRef, process, Pid, shutdown} -> + ok; + {'DOWN', _MRef, process, Pid, OtherReason} -> + {error, OtherReason} after Time -> exit(Pid, kill), %% Force termination. receive @@ -715,14 +727,11 @@ monitor_child(Pid) -> receive %% If the child dies before the unlik we must empty %% the mail-box of the 'EXIT'-message and the 'DOWN'-message. - {'EXIT', Pid, Reason} -> - receive - {'DOWN', _, process, Pid, _} -> - case Reason of - normal -> ok; - _ -> {error, Reason} - end - end + {'EXIT', Pid, Reason} -> + receive + {'DOWN', _, process, Pid, _} -> + {error, Reason} + end after 0 -> %% If a naughty child did unlink and the child dies before %% monitor the result will be that shutdown/2 receives a @@ -732,11 +741,8 @@ monitor_child(Pid) -> %% that will be handled in shutdown/2. ok end. - -check_shutdown_reason(Reason, Reason) -> ok; -check_shutdown_reason(_ , normal) -> ok; -check_shutdown_reason(Reason, _) -> {error, Reason}. - + + %%----------------------------------------------------------------- %% Child/State manipulating functions. %%----------------------------------------------------------------- -- cgit v1.2.1