[erlang-bugs] release_handler: cannot find top supervisor for application *

[Steve Vinoski]

On Thu, Mar 18, 2010 at 9:30 AM, Paul Guyot <pguyot@REDACTED> wrote:
> Hello,
>
> There seems to be an incompatibility between stdlib 1.16.5 and sasl 2.1.9 and earlier.
> This incompatibility was introduced with OTP-8324 (github commit 88b530ea24977081020feb2123124063e58dfc12). As a consequence, one might not be able to upgrade a root supervisor in release procedures, and releases produce warnings like :
>
> =ERROR REPORT==== 2010-03-18 08:04:50 ===
> release_handler: cannot find top supervisor for application mnesia
>
> (and it goes on for every application).
>
> Since commit 88b530ea24977081020feb2123124063e58dfc12, sys:get_status returns the formatted state from the module's format_status/2 function. For a supervisor, it means calling gen_server:format_status/2. As a result, release_handler:get_supervisor_module1/1 fails with a bad match.
>
> get_supervisor_module1(SupPid) ->
>  {status, _Pid, {module, _Mod},
>   [_PDict, _SysState, _Parent, _Dbg, Misc]} = sys:get_status(SupPid),
>  [_Name, State, _Type, _Time] = Misc,                                  <--- bad match happens here
>  %% Cannot use #supervisor_state{module = Module} = State.
>  {ok, element(#supervisor_state.module, State)}.
>
> Misc used to be a list with 4 elements before 88b530ea24977081020feb2123124063e58dfc12, it now is a list with three elements and the actual state of the supervisor is deep inside the third element.

Not trying to downplay the problems you've run into, but IMO the above
code strikes me as a fairly undesirable coupling between sasl, sys,
and gen_server.

> Moreover, sys:get_status calls gen_server:format_status which in turn calls Module:format_status if it exists and uses that as the last element of Misc list, and as a consequence, we cannot be sure how to get the state (in order to find out the supervisor callback module). In other words, a quick fix that would match against the new result of sys:get_status might fail for application root supervisors that implement a custom format_status/2.

I could be missing something, but wouldn't supervisor need to
implement its own format_status/2 that allowed its callback modules to
supply their own version of format_status/2? The gen_server code will
only call its direct callback module's format_status if it has one,
and supervisor doesn't have one.

> However, format_status/2 callback itself is not documented in supervisor(3) (it is documented in gen_server(3))....

Right, because supervisor doesn't support a custom format_status.

> I believe this deserves a rewrite (or a revert of 88b530ea24977081020feb2123124063e58dfc12).

Not sure a revert or rewrite is needed, but obviously that's
ultimately up to Björn and crew. IMO, it would be best to eliminate
the questionable coupling between sasl, sys, and gen_server, but that
would need work (which I'd be quite happy to do, given I'm the source
of the patches in question). But for a short term fix, perhaps this
patch would work:

diff --git a/lib/sasl/src/release_handler_1.erl
b/lib/sasl/src/release_handler_1.erl
index e3e3cab..6d6216d 100644
--- a/lib/sasl/src/release_handler_1.erl
+++ b/lib/sasl/src/release_handler_1.erl
@@ -554,7 +554,7 @@ get_supervisor_module(SupPid) ->
 get_supervisor_module1(SupPid) ->
     {status, _Pid, {module, _Mod},
      [_PDict, _SysState, _Parent, _Dbg, Misc]} = sys:get_status(SupPid),
-    [_Name, State, _Type, _Time] = Misc,
+    [_Header, _Data, {data, [{"State", State}]}] = Misc,
     %% Cannot use #supervisor_state{module = Module} = State.
     {ok, element(#supervisor_state.module, State)}.

Obviously this has just as much coupling as the original code, but
this isn't intended as an official patch. Paul, maybe you could try
it? Unfortunately I don't believe the sasl tests are released, so I
can't check them.

--steve