[erlang-bugs] Re: [erlang-bugs 8] Re: possible supervisor bug in r14b02

[Ingela Anderton Andin]

Hi!

We found that your patch was not complete and also a little buggy.
Normally we require a test case for patches, but as this was a
latent bug that was triggered by our latest change we
decided to write a test case our selfs.

We think the solution should look like follows and it
will be included in dev shortly.  (Diff is based on your patch)

index b511545..368dc2e 100644
@@ -344,8 +344,12 @@ handle_call({delete_child, Name}, _From, State) ->
handle_call({terminate_child, Name}, _From, State) ->
     case get_child(Name, State) of
     {value, Child} ->
-        NChild = do_terminate(Child, State#state.name),
-        {reply, ok, replace_child(NChild, State)};
+        case do_terminate(Child, State#state.name) of
+        #child{restart_type = temporary} = NChild ->
+            {reply, ok, state_del_child(NChild, State)};
+        NChild ->
+            {reply, ok, replace_child(NChild, State)}
+        end;
     _ ->
         {reply, {error, not_found}, State}
     end;
@@ -818,11 +822,11 @@ state_del_child(Child, State) ->
     State#state{children = NChildren}.
 
 del_child(Name, [Ch|Chs]) when Ch#child.name =:= Name, 
Ch#child.restart_type =:= temporary ->
-    [Chs];
+    Chs;
 del_child(Name, [Ch|Chs]) when Ch#child.name =:= Name ->
     [Ch#child{pid = undefined} | Chs];
 del_child(Pid, [Ch|Chs]) when Ch#child.pid =:= Pid, 
Ch#child.restart_type =:= temporary ->
-    [Chs];
+    Chs;
 del_child(Pid, [Ch|Chs]) when Ch#child.pid =:= Pid ->
     [Ch#child{pid = undefined} | Chs];
 del_child(Name, [Ch|Chs]) ->

Regards Ingela Erlang/OTP team - Ericsson AB

Filipe David Manana wrote:
> Thanks :)
>
> On Mon, Mar 28, 2011 at 9:24 AM, Ingela Anderton Andin
> <ingela@REDACTED> wrote:
>   
>> Hi!
>>
>> We will take a look at your patch it sounds like it is the right thing to
>> do.
>> Temporary processes should not be restarted so you should not have
>> to save their  init-arguments, although until the last release they where
>> saved
>> so it was possible to restart them! Especially for simple_one_for_one
>> supervisors
>> that may have lots of temporary processes memory consumption can
>> go sky high if you save them.
>>
>> Regards  Ingela Erlang OTP team - Ericsson AB
>>
>> Filipe David Manana wrote:
>>     
>>> Hi,
>>>
>>> In R14B02 I noticed that for a child with a "temporary" restart_type,
>>> we discard its A component of the MFA tuple when adding the childspec
>>> to the list of the supervisor's children [1].
>>>
>>> When the child terminates, its spec is never removed from the list of
>>> the supervisor's children specs.
>>> Then if we call supervisor:restart_child/2 after the child terminates,
>>> the handle_call clause for restart_child gets the childspec with an
>>> MFA  that is {M, F, undefined} [2]. At that point do_start_child will
>>> call apply(M, F, undefined) [3] which will cause the supervisor to
>>> reply with an error, instead of returning {ok, Pid} as in previous
>>> releases. An example for the returned error:
>>>
>>>  {error,{'EXIT',{badarg,[{erlang,apply,[gen_server,start_link,undefined]},
>>>                              {supervisor,do_start_child,2},
>>>                              {supervisor,handle_call,3},
>>>                              {gen_server,handle_msg,5},
>>>                              {proc_lib,init_p_do_apply,3}]}}}
>>>
>>> The patch at [4] fixes the issue for me. The particular code that is
>>> no longer working in R14B02 but worked on all previous releases, is
>>> from Apache CouchDB, see [5]
>>>
>>> This issue was introuced by OTP-9064 (reading from the R14B02 release
>>> notes).
>>> Was this intended behaviour? It doesn't make much sense for me to keep
>>> a temporary childspec in the supervisor once the child terminates, so
>>> I believe deleting it from the state is the right thing to do.
>>>
>>> [1] -
>>> https://github.com/erlang/otp/blob/dev/lib/stdlib/src/supervisor.erl#L787
>>> [2] -
>>> https://github.com/erlang/otp/blob/dev/lib/stdlib/src/supervisor.erl#L314
>>> [3] -
>>> https://github.com/erlang/otp/blob/dev/lib/stdlib/src/supervisor.erl#L246
>>> [4] -
>>> https://github.com/fdmanana/otp/commit/2697042aa9ebab2fcd208c93b7f454b25bc580d4
>>> [5] -
>>> https://github.com/apache/couchdb/blob/trunk/src/couchdb/couch_replicator.erl#L119
>>>
>>>
>>>       
>> _______________________________________________
>> erlang-bugs mailing list
>> erlang-bugs@REDACTED
>> http://erlang.org/mailman/listinfo/erlang-bugs
>>
>>     
>
>
>
>