[erlang-questions] simple_one_by_one trouble and rare start_link/2 gen_fsm

[Angel J. Alvarez Miguel]

Hi,

Please see example attached

While calling a simple_one_by_one supervisor to start a gen fsm I found it causes calling a start_link/2 function on the callback module of the gen_fsm.

I was not able to locate this start_link/2 on the docs and i dont know how Args are suppoused to be joined (those from the childspec on the supervisor and those from the start_child call.



Erlang R15B01 (erts-5.9.1) [source] [64-bit] [smp:4:4] [async-threads:0] [hipe] [kernel-poll:false]

[QuickCheck] [PropEr] [Erl0MQ]
Eshell V5.9.1 (abort with ^G)
1> test:main().
Starting simple_one_by_one group supervisor...
[GROUP SUP] Ready to manage MuC wuth opts: [{option1,40}]'s

Adding a child dynamically...

start_link called with 2 arguments: [{option1,40}] and [{dynamic_option,43}] this is not documented!!
init fsm with args: []
{ok,<0.34.0>}

Im alive!
Im alive!
Im alive!
Im alive!
Im alive!
Im alive!
FSM terminated (shutdown) after a few iterations on state idle...

2>

am I wrong or this is not documented (R15B1)?

/Angel

[Vance Shipley]

On Thu, Jun 21, 2012 at 12:30:08AM +0200, Angel J. Alvarez Miguel wrote:
} While calling a simple_one_by_one supervisor to start a gen fsm I found
} it causes calling a start_link/2 function on the callback module of the
} gen_fsm.

You are specifying that function to be called to start the worker yourself.
In your module group_supervisor.erl you provide a child_spec() in the return
value of init/1:

{mucfsm, {sim_group_fsm, start_link, [Opts]}, transient, 60, worker, [sim_group]}


The form of which is:

{Id, StartFunc, Restart, Shutdown, Type, Modules}

The second argument, StartFunc, has the form:

{Module, Function, Arguments}

You provided:

{sim_group_fsm, start_link, [Opts]}


--
-Vance
_______________________________________________
erlang-questions mailing list
erlang-q...@erlang.org
http://erlang.org/mailman/listinfo/erlang-questions

[Angel J. Alvarez Miguel]

That's right...

But start_link it is expected to be or arity 3 and Opts is a property list.

but then on test:main/0 when i do supervisor:start_child(SupPid,Opts2)

being Opts2 another property list, then suppervisor should call:

apply(sim_group_fsm,start_link,Opts ++ Opts2) on its line 319 which in turn calls gen_fsm:start_link, thus providing a Pid in the end...

but what it really does is apply(sim_group_fsm,start_link,Opts ++ Opts2,[ ]) and then crashes unless i declare a start_link/2 on my gen_fsm...

{
{badmatch,
{error,
{'EXIT',
{undef,
[
{sim_group_fsm,start_link,[[{option1,40}],[{option29,100}]],[]},
{supervisor,do_start_child_i,3,[
{file,"supervisor.erl"},
{line,319}
]}......

i dont see why i finish having a apply/4 call in supervisor: 319 instead of an apply/3...

I want the sup to propagate one property list to children specs while i will provide another during children spawns
providing that sup combines two in any way children will manage to demunge ...both property lists

So cant just see what im doing wrong....

/angel


PS: i changed a bit the test code to force sup crash

[Angel J. Alvarez Miguel]

i did'nt find any example of mixing args from the supervirsor childspec
and start_child(...) so im going to make my own digging in this issue:

Let see How the simple_one_by_one works...

handle_call({start_child, EArgs}, _From, State) when ?is_simple(State) ->
Child = hd(State#state.children),
#child{mfargs = {M, F, A}} = Child,
Args = A ++ EArgs,
case do_start_child_i(M, F, Args) of
<------>{ok, undefined} when Child#child.restart_type =:= temporary ->
<------> {reply, {ok, undefined}, State};
<------>{ok, Pid} ->
<------> NState = save_dynamic_child(Child#child.restart_type, Pid, Args, State),
<------> {reply, {ok, Pid}, NState};
<------>{ok, Pid, Extra} ->
<------> NState = save_dynamic_child(Child#child.restart_type, Pid, Args, State),
<------> {reply, {ok, Pid, Extra}, NState};
<------>What ->
<------> {reply, What, State}
end;


Shouldnt be do_start_child_i(M,F,EArgs) of the form :

do_start_child(M,F,[EArgs]) ??

or the inner call apply(M,F,[A])?

Provided the docs state that your unique exported funcion must be of arity one you should
expect that code will take measures in order to enforce arity one, enclosing whatever you pass it in a List on the apply phase.

The semanctics of apply use a list to KNOW how many args the target call has, while the
semantics of sup childspec + sup start_child use a list ++ op to know how many args will be passed
to your callback of arity ONE.

So for the apply part all your functions are arity ONE

Without the ability to join Args from the ChildSpec and from the start child call i would concur that you should
enclose in a list whatever you want to be as a sole argument and this code be ok, but as soon as you can put
two or more args one on the ChildSpec and one on the call Sup code must enforce that only one arg APPLY
call is made so you end calling start_link/1 whatever you pass on the args...

Even the DOCS state "[ term() ]" as the MFA on the ChildSpec


My stdlib-1.18.1.pdf says (Pag 369)

"start_child(SupRef, ChildSpec) -> startchild_ret()
Types:
SupRef = sup_ref()
ChildSpec = child_spec() | (List :: [term()])
...
...

...If the case of a simple_one_for_one supervisor, the child specification defined in Module:init/1 will be
used and ChildSpec should instead be an arbitrary list of terms List. The child process will then be started by
appending List to the existing start function arguments, i.e. by calling apply(M, F, A++List) where {M,F,A}
is the start function defined in the child specification."

Here IMHO lies the error, as A is a list we have [any()...] ++ [any()...] is a list [any()...,any()...] and thats get us an apply
call of arity > 1 ALWAYS even when you chilkdspec has a {M,F,[]} and you try provide a [..] in your start_child it will fail
unless both argslists are void.

So a combination of:

init(Opts) ->
io:format("[GROUP SUP] Ready to manage MuC with opts: ~p\'s\n",[Opts]),

{ok, {
{simple_one_for_one, 1, 60},
[
{mucfsm, {sim_group_fsm, start_link, [arg1,arg2]}, transient, 60, worker, [sim_group]}
]
}
}.


and a client that makes a call like:
...
{ok,ChildPid} = supervisor:start_child(SupPid,[arg3,arg4]),
...

Gets you a final apply call {M,F,A} where A is in the form [arg1,arg2,arg3,arg4]
not [[arg1,arg2,argf3,arg4]] at is should be when you intent to call a start_link/1....

Erlang R15B01 (erts-5.9.1) [source] [64-bit] [smp:4:4] [async-threads:0] [hipe] [kernel-poll:false]

[QuickCheck] [PropEr] [Erl0MQ]

Starting simple_one_by_one group supervisor...
[GROUP SUP] staring with argument: [{option1,40}]

Eshell V5.9.1 (abort with ^G)

1> [GROUP SUP] Ready to manage MuC with opts: [{option1,40}]'s
Adding a child dynamically...
{"init terminating in do_boot",{{badmatch,{error,{'EXIT',{undef,[{sim_group_fsm,start_link,[arg1,arg2,arg3,arg4],[]},{supervisor,do_start_child_i,3,[{file,"supervisor.erl"},{line,319}]},{supervisor,handle_call,3,[{file,"supervisor.erl"},{line,344}]},
{gen_server,handle_msg,5,[{file,"gen_server.erl"},{line,588}]},{proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,227}]}]}}}},[{test,main,0,[{file,"test.erl"},{line,11}]},{init,start_it,1,[]},{init,start_em,1,[]}]}}


/Angel

[Robert Virding]

I don't really think that there is an inconsistency here.

In all the cases where something is to be started, supervisor, application and proc_lib, the specifier for this is the Mod,Fun,Args of apply and spawn. Though I will admit that in the application case the function name is predefined, almost like a call back function. And for callbacks all the arguments, number and meaning, are predefined.

I personally think the main problems are:

- Inconsistency in the documentation caused by using "Args" to mean different things. So in supervisor. application and proc_lib "Args" really is a list of arguments in apply/spawn style where the call is built using the length of the list to determine the arity of the called function. In gen_XXX the ONE argument to Mod:init/1 is also called "Args" where it can be anything and even if it is a list it will only become one argument. Calling it "Data" would have made it easier to understand.

- The second argument supervisor:start_child/2 to completely different, both type and meaning, depending on whether the supervisor is a simple_one_for_one or anything else. Yes, I know that it is documented but it IS inconsistent.

I think that in general making all the start functions arity 1 and calling them with a list of arguments is of no real benefit. Apart from simple_one_for_one supervisors the arguments to the start function are anyway fixed so there would be no real gain here anyway.

Also it would be a completely backwards incompatible change which would cause a lot of rewriting, so I don't see it being applied.

We will just have to live with it. If it is of any consolation there are changes I would like to make which would also never be made. :-)

Robert

[Gustav Simonsson]

This is a post talking about why you should not name things "Data" :P
http://petdance.com/2012/04/the-worlds-two-worst-variable-names/

Perhaps a better name for what you send to Mod:init/1 in gen_XXX is "Argument"
so it's more clear from the variable name that it is a single term.

// Gustav

Sent from my PC

[Robert Virding]

In that case "InitData" is probably a better name than "Argument", which says about as much as "Data". Like the post though.

Anything but "Args" as ther is realy only one of it.

[Angel J. Alvarez Miguel]

How much is used this feature? How many people use it ?

I dont think so many, after a couple of greps along my installed OTP packages i would say i dont think anyone is using
this simple_one_for_one feaure as all MFA's in child specs seem to have the A part as [ ] so the args count in fixed by what you
put on start_child/2. The best i cant say in my limited knowledge is that this feature is nullified buy this fact so apparently almost
nobody is using it, hence the dumb patch i did.

However i didnt meant having all callbacks as init/1 but in this case provided that start_child can have arbitrary args list
this would make your posible callbacks arity any, at any time, upon what you put on the args.

this is error prone at least

In the best case scenario you can pass both args surrounded in list so you get a Module:init/2 callback to be called.... better
than nothing.

Do in the end maybe it is worth check if spec args are [] (the usual case) and keep doing a simple ++ but in other case do a better list joint
and keep your callback arity sane as documented...

I think it deserve at least be documented so other guys dont get stuck with this on the future .


/Angel

Ok ok, at least im not completely wrong, ill try to overcome this with other
approach like passing a pid of something else so children can ask some adult
whatever they know about their new playground.

Maybe things like this deserve a place in a TODO list for a future rewrite or
a OTP next-generation saga....

As always the good side of the history is a better understanding on the inner
working of the OTP beats.

Thanks Angel

[Robert Virding]

I agree on the documentation. It is also tricky to teach. :-)

I honestly have no idea how often simple_one_for_one is actually used. Or if people use the other restart types and use start_child/2 with a child spec. It would be interesting to find out.

Robert

----- Original Message -----
>

[Siri Hansen]

Hi Angel!

Thanks for pointing out this problem!

While one might agree that the design decision was not very good here, it is, as Robert points out, a totally backwards incompatible change you wish to do. And even though simple_one_for_one is not the most used supervisor type, it is for sure used and it is not an option for us to do such a change.

However, I do agree that the documentation could be better here, and I will write a ticket to make sure the issue is not forgotten. As always, a user contribution might very well make this change happen faster than our own priorities else would allow :)

Best regards

/siri@otp

2012/6/22 Angel J. Alvarez Miguel <cl...@uah.es>