Michał,
I have talked to James and he has pointed out that removing start_link may
severely complicate code reloading. The issue with not having a step
between child_spec and start_link is that any change in the child_spec
won't be propagate until the supervisor restarts or is code upgrade.
As an example, let's consider the following code in Phoenix:
def child_spec(arg) do
GenServer.child_spec(__MODULE__, arg, [])
end
And then you change it to:
def child_spec(arg) do
GenServer.child_spec(__MODULE__, {arg, arg+1}, [])
end
def init({..., ...}) do
Now if the GenServer crashes, the supervisor still have the old version of
the child_spec, and it will attempt to restart the old version.
You really want to protect the interaction between client-servers into a
proper API and don't have the child spec reach directly into the server.
This is not an issue only with Phoenix but with any kind of code reloading
/ hot code swapping.
Therefore, we want to keep an entry point, such as start_link/1, static as
much as possible and the current proposal pushes towards that direction. My
advice still remains though: please look at both the current and your
proposal with those new insights and see what we can make out of it.
*José Valim*
www.plataformatec.com.br
Skype: jv.ptec
Founder and Director of R&D
On Tue, Feb 21, 2017 at 9:53 PM, José Valim <[email protected]
> wrote:
> By removing the application callbacks we remove the primary place of
>> injecting dynamic configuration into an application. Dynamic configuration
>> is already a major hurdle, without this proposal or with it, but I'm afraid
>> this will only worsen the situation.
>>
>
> The best place for dynamic configuration is inside the init/1 function. We
> have recently added an init/1 function to Ecto.Repo and we will add one to
> Phoenix.Endpoint before 1.3 is out. So the plan is to push people towards
> loading those values inside init/1.
>
> So I think this proposal will help towards the proper direction, as it
> will be one less wrong place to put runtime configuration.
>
>
>> I can see at least three issues here:
>>
>> * the __MODULE__ part feels repetitive - there should be a way to specify
>> it only in one place.
>> * we need to know if the behaviour should be a worker or a supervisor
>> child.
>> * in practice it might be hard to distinguish what are the arguments to
>> the callback itself, and what are the arguments to the behaviour (e.g.
>> name). Now I need to understand the flow of the arguments through 3
>> functions, instead of two. Where before there was only start_link and init
>> (and it was sometimes already confusing), now we have child_spec,
>> start_link and init - even more confusion awaits.
>>
>
> I want to point out that you forgot to consider the worker/supervisor
> helpers and the parent supervisor in your analysis of how it works today
> (before this proposal). 1) __MODULE__ is repeated in worker/supervisor and
> start_link. 2) You need to know if it is a worker/supervisor. And 3) you
> need to understand the flow of args from worker/supervisor to start_link
> and to init.
>
> In other words, if you do not want to generate the child_spec
> automatically, which is one of the reasons for this proposal, then it is
> going to be as repetitive as it is today (not worse).
>
> Something that I did not mention is that we will likely add functions such
> as GenServer.child_spec/2 to help cases where folks need to generate
> child_spec/2 manually:
>
> def child_spec(arg) do
>
> GenServer.child_spec(__MODULE__, args: [arg]) # :id, :type, etc are
> also options
>
> end
>
>
> which also solves 1 and 2.
>
> Edit: I can see that you propose something similar as well. :)
>
>
>> Additionally we could define the child spec in a way that does not
>> require the start_link function.
>>
>
> We have discussed this extensively and there are some unresolved issues.
> For example:
>
> 1. How would you pass the :name to your GenServer?
> 2. What about all other GenServer.start_link/3 options?
> 3. How will you start the server in tests or anywhere else if it has no
> start_link function?
>
> Our main concern with mixing child_spec and start_link is that it would
> ultimately mix options and configurations that apply at distinct stages.
> Even if we introduce something such as GenServer.child_spec(module, arg,
> start_opts, child_opts), that solves 1 and 2, we still need to find good
> answers for 3. For tests, we could provide start_child/1 that starts a
> process under a supervisor for that specific test. But we still need to
> handle ad-hoc cases. Should we say processes can only be started under a
> supervisor? Would that be enough?
>
>
>> Furthermore, it addresses the issue of defining a "start_link" function -
>> we never call it explicitly, it's not a callback of any behaviour, and
>> seems quite random as a name (the BIF to start a process is called
>> spawn_link).
>>
>
> spawn_link and start_link have different return signatures.
>
> Notice your proposal won't make start_link disappear as well, since they
> still need to exist as an entry point in any abstraction such as Agent,
> GenServer, etc. For example, if we say you can only start processes under a
> supervisor per point 3 above, we still need a mechanism to quickly start a
> supervisor in IEx, which will likely be Superivsor.start_link.
>
> We should also consider, requiring calling the child_spec function
>> directly in the children specification, instead of defaulting to calling
>> child_spec([]) - it will have similar discoverability issues that current
>> start_link has, and is only slightly more verbose.
>>
>
> If you call child_spec directly in the children specification, then we are
> going to have the same problems as today in that you are no longer able to
> perform hot code upgrades as the configuration is loaded outside of the
> supervisor, which is one of the issues this proposal aims to solve.
>
> Of course we would like to require as little as possible when implementing
> behaviours but there are many trade-offs involved in the process.
>
> In any case, thanks for the counter proposal. It brings very good points
> Michał! I would suggest for you to revisit your proposal and consider the
> following points:
>
> * How should GenServer.child_spec look like if we want to pass both
> start_link options and child_spec options?
>
> * How to start processes if we don't have a start_link function. Is the
> assumption they can only be supervised enough? (unless you call the
> underlying start_link function in the abstraction directly)
>
> * You claim we will be able to get rid of start_link but I don't think
> that's true, at least not for abstractions such as GenServer and
> Supervisor. How does that impact your proposal?
>
> * Finally you say there are no hidden defaults but everything you hide
> between GenServer.child_spec is a hidden default. Someone will eventually
> have learn about it when they need to write their own child_spec. Does it
> change anything?
>
> Let us know how it will progress. Meanwhile we will also discuss the
> points above between us too. We will ping you if we come up with any new
> development.
>
>
--
You received this message because you are subscribed to the Google Groups
"elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4JSo2_Bag%2BUhRQpJkQM0NZz8gngFZJvT5bAn_g2M2MV3w%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
