[erlang-questions] release_handler and documentation of supervisor's child spec

[Serge Aleynikov]

To: OTP maintainers of the 'supervisor' documentation and SASL's release 
handler.

1. The documentation of child specification of a supervisor states:

<quote chapter="Design Principles">
{Id, StartFunc, Restart, Shutdown, Type, Modules}
     ...
     Modules = [Module] | dynamic
         Module = atom()

Modules should be a list with one element [Module], where Module is the 
name of the callback module, if the child process is a supervisor, 
gen_server or gen_fsm. If the child process is a gen_event, Modules 
should be dynamic.
</quote>

<quote chapter="stdlib/supervisor/Supervision Principles">
Modules is used by the release handler during code replacement to 
determine which processes are using a certain module. As a rule of thumb 
Modules should be a list with one element [Module], where Module is the 
callback module, if the child process is a supervisor, gen_server or 
gen_fsm. If the child process is an event manager (gen_event) with a 
dynamic set of callback modules, Modules should be dynamic. See OTP 
Design Principles for more information about release handling.
</quote>

What neither reference mentions is the fact that if the child process is 
*not* a supervisor, gen_server, gen_fsm, nor gen_event, what the 
requirements are for the process when the Modules parameter is set to 
'dynamic'.  An example could be a module implemented using proc_lib. 
Unless implemented correctly release_handler won't be able to upgrade it 
properly.

It looks like the requirement is that the supervised process must 
implement get_modules/0 function that should return a list of modules 
(with at least one item) it executes as a call_back or process module.

2. The release_handler_1:get_procs/2 does the following call:

get_procs([{Name, Pid, worker, dynamic} | T], Sup) when pid(Pid) ->
     Mods = get_dynamic_mods(Name),
     ...

Shouldn't this instead be the following?

     Mods = get_dynamic_mods(Pid),
     ...

The current implementation mandates that the registered name of a 
dynamic process must match the supervisor's chiled specification ID 
(which is a name that is used to identify the child specification 
internally by the supervisor).  This mandate is somewhat odd for custom 
processes implemented using proc_lib.

To illustrate this point, I had the following code in the LAMA 
application on jungerl:

init([AlarmOptions, SyslogOpts]) ->
     SafeSupervisor =
         {lama_safe_sup,
             {supervisor, start_link,
                 [{local, lama_sup_safe}, ?MODULE,
                  {safe, AlarmOptions, SyslogOpts}]},
             permanent, infinity, supervisor, [?MODULE]},
     %% Reboot node if lama_logger or lama_alarmer or lama_snmp_trapper 
crashes!
     {ok, {_SupFlags = {one_for_one, 0, 1}, [SafeSupervisor]}};

init({safe, AlarmOptions, SyslogOpts}) ->
     SyslogH =
         {lama_syslog_sup,
             {lama_guard, start_link,
                 [lama_guard_syslog, lama_syslog_h, SyslogOpts]},
             permanent, 2000, worker, dynamic},
     AlarmH =
         {lama_alarm_sup,
             {lama_guard, start_link,
                 [lama_guard_alarm, lama_alarm_h, AlarmOptions]},
             permanent, 2000, worker, dynamic},
     {ok, {_SupFlags = {one_for_one, 4, 3600}, [SyslogH, AlarmH]}}.

The lama_guard:start_link/1 is implemented like this:

start_link(GuardName, HandlerModule, Options) ->
     Self = self(),
     proc_lib:start_link(?MODULE, init, [GuardName, HandlerModule, 
Options, Self], infinity).

init(GuardName, HandlerModule, Options, Parent) ->
     case catch erlang:register(GuardName, self()) of
     true ->
         ....

We see here that the registered name of the lama_guard process is 
determined dynamically by the GuardName parameter as the same module is 
reused for two implementations.  This presents a problem with 
'lama_syslog_sup' and 'lama_alarm_sup' child specification IDs above if 
we don't give the same name to the supervisor's child spec as the name 
registered by the spawned processes 'lama_guard_syslog' and 
'lama_guard_alarm'.  So we have to rewrite the spec as:

     SyslogH =
         {lama_guard_syslog,
          ^^^^^^^^^^^^^^^^^
             {lama_guard, start_link,
                 [lama_guard_syslog, lama_syslog_h, SyslogOpts]},
                  ^^^^^^^^^^^^^^^^^
             permanent, 2000, worker, dynamic},
     AlarmH =
         {lama_guard_alarm,
          ^^^^^^^^^^^^^^^^
             {lama_guard, start_link,
                 [lama_guard_alarm, lama_alarm_h, AlarmOptions]},
                  ^^^^^^^^^^^^^^^^
             permanent, 2000, worker, dynamic},

Since ID in the supervisor's child specification is documented as "used 
to identify the child specification internally by the supervisor", it 
shouldn't have any implications on the choice of registered names of 
supervised processes.

I don't know all the implications of changing

     Mods = get_dynamic_mods(Name),
to
     Mods = get_dynamic_mods(Pid),

in the release_handler_1:get_procs/2, but it looks like the right thing 
to do in addition to documenting get_modules/0 requirement.

Regards,

Serge