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

[Filipe David Manana]

Oh, yes, I definitely made a mistake by putting the rest of the
children list inside another list. Copy-paste curse :)
Looks fine to me.

Thanks Ingela.

On Mon, Apr 4, 2011 at 1:30 PM, Ingela Anderton Andin
<ingela@REDACTED> wrote:
> 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
>>>
>>>
>>
>>
>>
>>
>
> _______________________________________________
> erlang-bugs mailing list
> erlang-bugs@REDACTED
> http://erlang.org/mailman/listinfo/erlang-bugs
>



-- 
Filipe David Manana,
fdmanana@REDACTED, fdmanana@REDACTED

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."