Proposal: Streamlining child specs

[José Valim]

Note: this proposal is also available in a gist - https://gist.github.com/josevalim/82fb1d20c9738bb3cc96449e67407df6

Streamlining child specs

Hello everyone,

This is a proposal for improving how supervisors are defined and used in Elixir. Before we go to the improvements, let's discuss the pain points of the current API.

The current state of the art

Module-based supervisors

Supervisors in Elixir can be defined using modules:

defmodule MySupervisor do
  use Supervisor

  def start_link do
    Supervisor.start_link(__MODULE__, [])
  end

  def init([]) do
    children = [
      worker(Child, [])
    ]

    supervise(children, strategy: :one_for_one)
  end
end

Notice we introduced the functions worker and supervise, which are part of the Supervisor.Spec module, to take care of defining the tuples used behind the scenes.

However, the above is still verbose for the cases we want to simply start a supervisor. That's when we introduced callback-less supervisors.

Callback-less supervisors

The idea behind callback-less supervisors is that you can start them directly without needing callbacks:

import Supervisor.Spec

children = [
  worker(Child, [])
]

Supervisor.start_link(children, strategy: :one_for_one)

While the example above is an improvement compared to previous versions, especially the early ones that required tuples to be passed around instead of using the worker/supervisor functions, it has the following issues:

• Importing Supervisor.Spec which exists in a separate module from Supervisor is confusing in terms of learning and exploring the API and documentation

• Children inside callback-less supervisors cannot be hot code upgraded, since the definition is provided before the supervisor even starts

   * The worker(Child, []) API is confusing in regards to which function it will invoke and with which arguments. The fact worker(Child, [1, 2, 3]) invokes Child.start_link(1, 2, 3) which is then packed into an argument given to GenServer.start_link/3 start is not helpful.

Application integration

Perhaps the most troubling aspect of supervisors is the amount of code necessary to integrate any supervised process into a new application. For example, imagine you have created a new application with mix new my_app and after a while you decide it needs an agent. In order to correct supervise your agent implenentation, you will need to:

1. Define a new module that use Application and implements the start/2 callback which starts a supervisor with the agent as a worker

   defmodule MyApp do
     use Application
   
     def start(_type, _args) do
       import Supervisor.Spec
   
       children = [
         worker(MyAgent, [], restart: :transient)
       ]
   
       Supervisor.start_link(children, strategy: :one_for_one)
     end
   end

    2. Make sure to define the `:mod key in the mix.exs:

      elixir def application do [mod: {MyApp, []}] end    

The amount of code to go from unsupervised to supervised often lead people down the wrong road of not adding supervision at all. Another sign the code above is a common boilerplate is that it has been automatized under the mix new my_app --sup flag. The --sup flag has been helpful but it is useless in projects you have already created.

The other problem with the code above is that the agent configuration ends-up spread through multiple files. Properties such as :shutdown and its type (worker or supervisor) is most times better to be co-located with its implementation rather than specified separately in the supervision tree. Specifying it at the supervision tree is useful only in cases the same implementation is started multiple times under the same or different trees, which is not the most common scenario.

A streamlined solution

In the previous section we have explored the current state of application and supervisors in Elixir. In this section, we propose a streamlined solution that is going to touch all of the problems outlined above, starting with the application one.

Default application callbacks

Ideally, we want to make it as straight-forward as possible to go from non-supervised to supervised code. If most applications callbacks end-up having the exact same structure, such boilerplate can likely be addressed by Elixir.

Therefore, instead of forcing users to define an application callback with a boilerplate supervision tree, Elixir could ship with the application definition where we only need to tell Elixir which processes to supervise:

def application do
  [extra_applications: [:logger],
   supervise: [MyAgent, {Registry, name: MyRegistry}]]
end

The :supervise option allows us to specify a list of module names or tuples with two elements, where the first element is the module name and second element is any argument. When the application starts, Elixir will traverse the list invoking the child_spec/1 function on each module, passing the arguments.

In other words, we will push developers to co-locate their supervision specification with the module definition.

The child_spec/1 function

The heart of the proposal is in allowing modules to specify the supervision specification for the processes they implement. For example, our agent definition may look like this now:

defmodule MyAgent do
  def child_spec(_) do
    %{id: MyAgent, start: {__MODULE__, :start_link, []}, restart: :transient}
  end

  # ...
end

With the definition above, we are now able to use MyAgent as a supervised process in our application, as shown in the previous section.

If your application requires multiple supervisors, Supervisors can also be added to the application's :superviseoption, as long as they also implement the child_spec/1 function. Furthermore, notice the Supervisor API, such as start_link/2 will also be updated to allow a list of modules, in the same format as the application's :supervise:

defmodule MySup do
  def child_spec(_) do
    %{id: MySup, start: {__MODULE__, :start_link, []}, type: :supervisor}
  end

  def start_link do
    Supervisor.start_link([OtherAgent], strategy: :simple_one_for_one, name: MySup)
  end
end

Besides keeping the agent configuration in the same module it is defined, the module based child_spec/1 has a big advantage of also being hot code swap friendly, because by changing the child_spec/1 function in the MyAgentmodule, we can patch how the agent runs in production.

Not only that, by colocating child_spec/1 with the code, we make it simpler to start abstractions such as Phoenix.Endpoint or Ecto.Repo under a supervision tree, as those abstractions can automatically define the child_spec/1 function, no longer forcing developers to know if those entries have the type worker or supervisor.

Today developers likely have the following code in their Phoenix applications:

children = [
  supervisor(MyApp.Endpoint, []),
  supervisor(MyApp.Repo, [])
]

which constraints both Ecto and Phoenix because now they are forced to always run a supervisor at the top level in order to maintain backwards compatibility. Even worse, if a developer add those entries by hand with the wrong type, their applications will be misconfigured.

By storing the child_spec directly in the repository and endpoint, developers only need to write:

supervise: [MyApp.Endpoint, MyApp.Repo]

which is cleaner and less error prone.

Catching up with OTP

In the previous section we have mentioned that Supervisor.start_link/2 will also accept a list of modules or a list of tuples with two elements on Supervisor.start_link/2. In such cases, the supervisor will invoke child_spec/1appropriately on the list elements.

You may have also noticed we have used maps to build the child_spec/1 functions:

def child_spec(_) do
  %{id: MySup, start: {__MODULE__, :start_link, []}, type: :supervisor}
end

The reason we chose a map with the keys above is to mirror the API introduced in Erlang/OTP 18. Overall, Supervisor.start_link/2 expect a list of children where each child must be:

• an atom - such as MyApp. In such cases, MyApp.child_spec([]) will be invoked when the supervisor starts
• a tuple with two elements - such as {MyApp, arg}. In such cases, MyApp.child_spec(arg) will be invoked when the supervisor starts
• a map - containing the keys :id (required), :start (required), :shutdown, :restart, :type or :modules

Introducing maps will allow a more natural and data-centric approach for configuring supervisors, no longer needing the helper functions defined in Supervised.Spec today.

A practical example

In order to show how the changes above will affect existing applications, we checked how applications generated by Phoenix would leverage them. The answer is quite positive. The whole lib/demo.ex file will no longer exist:

defmodule Demo do
  use Application

  # See http://elixir-lang.org/docs/stable/elixir/Application.html
  # for more information on OTP Applications
  def start(_type, _args) do
    import Supervisor.Spec

    # Define workers and child supervisors to be supervised
    children = [
      # Start the Ecto repository
      supervisor(Demo.Repo, []),
      # Start the endpoint when the application starts
      supervisor(Demo.Endpoint, []),
      # Start your own worker by calling: Demo.Worker.start_link(arg1, arg2, arg3)
      # worker(Demo.Worker, [arg1, arg2, arg3]),
    ]

    # See http://elixir-lang.org/docs/stable/elixir/Supervisor.html
    # for other strategies and supported options
    opts = [strategy: :one_for_one, name: Demo.Supervisor]
    Supervisor.start_link(children, opts)
  end
end

instead it will be replaced by:

def application do
  [extra_applications: [:logger],
   supervise: [Demo.Repo, Demo.Endpoint]]
end

which Mix will translate behind the scenes to:

def application do
  [extra_applications: [:logger],
   mod: {Application.Default, %{supervise: [Demo.Repo, Demo.Endpoint]}}]
end

Overall we propose the following additions to def application:

• :supervise - expects a list of module names to supervise when the application starts (maps as childspecs are not supported as that would make them compile time)
• :max_restarts - the amount of restarts supported by application supervision tree in :max_seconds (defaults to 1)
• :max_seconds - the amount of time for :max_restarts (defaults to 5)

Those options will be handled by the default application module implemented in Application.Default. Mix will error building the application file if any of the options above are given alongside :mod.

The application tree supervisor will have strategy of :one_for_one and such is not configurable. If you need other supervision strategies, then all you need to do is define your own supervisor with a custom strategy and list it under :supervise in def application.

At the end of the day, the changes above will render both mix new --sup and Supervisor.Spec obsolete, as we now co-locate the child_spec/1 with the module definition, leading to better code and hot code upgrades, also making it easier to move from non-supervised to supervised applications.

Automating child_spec/1

We have explicitly defined the child_spec/1 function in all of the examples above. This poses a problem: what if someone defines a Supervisor module and forget to mark its type as :supervisor in the specification?

Given the Supervisor module already knows sensible defaults, we should rely on them for building specs. Therefore, we also propose use Agent, use Task, use Supervisor and use GenServer automatically define a child_spec/1function that is overridable:

defmodule MyAgent do
  use Agent, restart: :transient

  # Generated on: use Agent
  # def child_spec(opts) do
  #   %{id: MyAgent, start: {__MODULE__, :start_link, [opts]}, restart: :transient}
  # end

  def start_link(opts) do
    Agent.start_link(fn -> %{} end, opts)
  end

  ...
end

The default child_spec/1 will pass the argument given to child_spec/1 directly to start_link/1. This means MyAgentcould be started with no name in a supervision tree as follows:

supervise: [MyAgent]

and with a given name as:

supervise: [{MyAgent, name: FooBar}]

In other words, the changes in this proposal will push developers towards a single start_link/1 function that will receive options as a single argument. This mirrors the API exposed in GenServer, Supervisor and friends, removing the common confusion with passing more than one argument to start_link.

Of course, any of the defaults above, as well as the rare cases developers wants to dynamically customize child_spec/1 can be achieved by overriding the default implementation of child_spec/1.

Summing up

This proposal aims to streamline the move from non-supervised applications to supervised applications. To do so, we introduced the ability to colocate the child_spec/1 definition with the module implementation, which leads to more self-contained abstractions, such as Ecto.Repo and Phoenix.Endpoint. Those changes also allowed us to overhaul the supervision system, getting rid of Supervisor.Spec and making it more data-centric.

While those changes remove some of the importance given to application callbacks today, we believe this is a positive change. There is no need to talk about application callbacks if the majority of developers are using application callbacks simply to start supervision trees. The application callback will still remain import for scenarios where custom start, config_change or stop are necessary, such as those relying on included applications.

Since Elixir is a stable language, all of the code available today will continue to work, although this proposal opens up the possibility to deprecate Supervisor.Spec and mix new --sup in a far ahead future, preparing for an eventual removal in Elixir 2.0.

Thanks to Dave Thomas for initiating the proposal and the Elixir team for further discussions and reviews. Also thanks to Chris McCord and Saša Jurić for feedback.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

[Louis Pilfold]

Hello!

Very interesting. It does seem like this would be easier to learn :)

Little nitpick: I find the name child_spec/1 a little confusing. I would expect it to be the spec of the children of a supervisor defined by the module, due to the word "child".

    MySup.child_spec([])

    #=> spec of child of MySup

    MySup.spec([])

    #=> spec of MySup

Perhaps I just feel this way because of familiarity with the current system. 

Cheers,

Louis

--
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 elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4J1mmJQ_QUML5VeSnT81kprkTWBtjekD3hpHMUhVDJBZg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[José Valim]

>  I find the name child_spec/1 a little confusing

I read child_spec/1 as: the specification to run this as a child in a supervisor. Does that help?

I think calling it only "spec" can be even more confusing, as we also have typespecs.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-core+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4J1mmJQ_QUML5VeSnT81kprkTWBtjekD3hpHMUhVDJBZg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--

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 elixir-lang-core+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CABu8xFBTW6GcaCFSJ_z6KYzRN%3Dxt9tdRpxq6woXBcapz4st%2B5w%40mail.gmail.com.

[Michał Muskała]

This is a very well prepared proposal. I have one question and one further proposal to improve it.

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. It's difficult (and inconsistent in releases vs running with mix) to configure applications with dynamic values - be it from environment variables or external services like zookeeper. Is there a planned proposal to improve upon this (and I should stop thinking about it) or are the application callbacks still the best place for this? If so, how do we make it more obvious?

This is mostly an open question and I don't have a good answer for today, but I'd like to know where we stand.

Now let's see, how I think the proposal could be improved.

How would an example GenServer callback module will look like (without the proposed "automatic child_spec" and with the new GenServer proposal [1]):

defmodule MyServer do

  @behaviour GenServer

  def child_spec(_args) do

    %{id: MyServer, start: {__MODULE__, :start_link, []}, restart: :transient}

  end

  def start_link(args) do

    GenServer.start_link(__MODULE__, args, args)

  end

  def init(args) do

    {:ok, new_state(args)}

  end

  # ...

end

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. 

The "automatic child_spec" part of the proposal addresses the first and second issues, but unfortunately it introduces a lot of complexity and additional macro "magic". With the current work in GenServer to migrate away from overridable functions, I'm rather surprised by this. Additionally this does not address the third, even more important issue. Hiding things behind macros is not reducing the complexity - even worse it is hiding it. It makes defining the module "simple", but unfortunately not "easy"[2].

I propose we embrace the child_spec fully and use it to replace start_link entirely. Instead of providing a raw map in child_spec, we would call a child_spec function in the parent behaviour, delegating the responsibility of knowing if this should be a "supervisor" or "worker" similar to the "automatic child_spec" proposal, which was one of the main issues it embarked to solve. Additionally we could define the child spec in a way that does not require the start_link function.

How would a module implemented with this look like?

defmodule MyServer do

  @behaviour GenServer

  def child_spec(_args) do

    GenServer.child_spec(__MODULE__, args)

  end

  def init(args) do

    {:ok, new_state(args)}

  end

  # ...

end

To define a child supervisor, we could use:

Supervisor.child_spec([MyServer.child_spec([])], strategy: :simple_one_for_one, name: MySup)

Notice how the start_link function is gone. It is in no way more complex than the current status quo - there are still only two functions, but we don't need to know if the process is a worker or a supervisor. Additionally, there's only one place, where we define what module the behaviour will be using. 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). Where is it coming from? You need to know that start_link is this special snowflake of a function. In this proposal we explicitly call the child_spec function in the supervisor, and directly enter the behaviour's callbacks. It's very clear to trace the path of the code and understand what is happening - in the supervisor we call the MyServer.child_spec/1 function, which calls the GenServer.child_spec/2, which causes us to enter the behaviour with init/1 - there are no hidden defaults anywhere, no magic function names, and no macros (!). 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.

[1] https://github.com/elixir-lang/elixir/pull/5676

[2] https://www.infoq.com/presentations/Simple-Made-Easy

[Michał Muskała]

On 21 Feb 2017, 20:10 +0100, Louis Pilfold <lo...@lpil.uk>, wrote:

Hello!

Very interesting. It does seem like this would be easier to learn :)

Little nitpick: I find the name child_spec/1 a little confusing. I would expect it to be the spec of the children of a supervisor defined by the module, due to the word "child".

    MySup.child_spec([])

    #=> spec of child of MySup

    MySup.spec([])

    #=> spec of MySup

Perhaps I just feel this way because of familiarity with the current system. 

Cheers,

Louis

 

It's worth noting that the "child_spec" is already used in many places - cowboy, db_connection, poolboy (and probably more) all define a child_spec function.

Michał.

[Louis Pilfold]

Aye, that's true. An alternative could be `supervision_spec/1`?

Fishcakez just pointed out to me that Ecto, Phoenix and Plug already use `child_spec` in this way, probably best to stick to that convention. 

Cheers,

Louis

[José Valim]

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.

[José Valim]

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

[Myron Marston]

I definitely like this proposal overall.  There are some nuances I don't understand yet, but it sounds like y'all are asking the right questions.

In our code base we've already been defined a function like `child_spec` a few times for this exact purpose, so it neatly aligns with how I'd like to do things.  We've been calling the function `supervisor_spec` (e.g. it's the function to get the spec for a supervisor to supervise a process defined by this module) instead of `child_spec` and I think that's a better name -- but it sounds like there's some inertia around `child_spec` from other projects.

Myron

[Michał Muskała]

Thank you for the questions, José.

I think the child_spec could take arguments that are a merged form of both arguments to start_link and to the supervisor spec. How would it look like?

GenServer.child_spec(__MODULE__, arg: [arg], restart: transient, name: Foo)

Of course the options could be passed from "upstream". The options don't clash in the current behaviours. Of course it's possible they would in some custom behaviour or a future core one, but I think it should be fairly easy to solve those conflict on a case-by-case basis.

Starting without a supervisor is indeed a possible concern. That said, it's generally advised not to do this in regular code - the cases where starting a process outside of a supervision tree are rather rare and encountered by advanced users. Nothing is stopping them from defining the start_link function like we do today - but for majority of cases it's simply not needed, if not plain wrong to start processes outside of supervision tree.

There remain two places where starting ad-hoc processes outside of supervision trees is common - tests and iex sessions. A per-test supervisor you proposed (and an iex session supervisor) with a helper function to start the "child spec" under it might be a good idea. It will also solve one of the issues with tests and asynchronous termination of linked processes. By making this supervisor an all_for_one supervisor, the current behaviour of test termination in case of process failure will be maintained.

I don't see much difference (in usability) between: MyServer.start_link or start_child(MyServer) called from the test or iex.

> 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?

I think it is true. Let's see an example: here's the application from the mix & OTP tutorial refactored to take advantage of the proposal. No start_link required. I also added some comments to explain the code.

https://github.com/michalmuskala/kv_umbrella/commit/62bc49d21bbb93fa4e926ad7f7f41388fc978f83

Additionally here are the tests refactored to take into account the start_child proposal:

https://github.com/michalmuskala/kv_umbrella/commit/753680541f1384c6d756a6550fc55089c507818d

> 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?

You make a good point that "child_spec" will become a default. I admit, it will become a convention, as much as start_link is one right now. I think it still has merits, though. I also understand that because of the code reloading concerns we can't make the calls to "child_spec" explicit - it needs to be called inside the supervisor. It is my understanding that if the "child_spec" call is kept inside the supervisor, the code reloading will work perfectly fine. I might be missing something, though - code loading is a part of OTP I'm not terribly familiar with.

Michał.

[José Valim]

I think the child_spec could take arguments that are a merged form of both arguments to start_link and to the supervisor spec. How would it look like?

GenServer.child_spec(__MODULE__, arg: [arg], restart: transient, name: Foo)

Unfortunately this does not solve the problems mentioned in my last e-mail. :(

If I change the name: Foo when calling child_spec, it won't be reflected until the *supervisor* is restarted. However, if we have two distinct steps, child_spec and start_link, anything done in start_link is reloaded.

Because those two abstractions happen in very distinct moments and are called by different processes, I really really think we cannot mix the two different concerns without introducing drawbacks. Those drawbacks were not clear initially though, we had to think it through after your proposal. :)

 

I think it is true. Let's see an example: here's the application from the mix & OTP tutorial refactored to take advantage of the proposal. No start_link required. I also added some comments to explain the code.

https://github.com/michalmuskala/kv_umbrella/commit/62bc49d21bbb93fa4e926ad7f7f41388fc978f83

Additionally here are the tests refactored to take into account the start_child proposal:

https://github.com/michalmuskala/kv_umbrella/commit/753680541f1384c6d756a6550fc55089c507818d

To be clear, I agree we would be able to remove start_link from *your codebase*. However, you can't remove it from Supervisor, GenServer and friends because there are many situations you would still need them (for example, to start a Supervisor in IEx that will allow you to start a given child_spec.

[Michał Muskała]

On 23 Feb 2017, 10:37 +0100, José Valim <jose....@plataformatec.com.br>, wrote:

To be clear, I agree we would be able to remove start_link from *your codebase*. However, you can't remove it from Supervisor, GenServer and friends because there are many situations you would still need them (for example, to start a Supervisor in IEx that will allow you to start a given child_spec.

 

Oh, yes. I think it's fine to keep start_link on the behaviours. My aim was to remove it from callback modules exclusively.

I also understand the issue with supervisors better now, and I see where is the issue - but I think this affects both the original proposal and my modified proposal in a similar way.

[José Valim]

> I also understand the issue with supervisors better now, and I see where is the issue - but I think this affects both the original proposal and my modified proposal in a similar way.

Right. The issue also exists in the current proposal for any parameter that is passed via the child_spec. The trouble with your proposal is that the only option is to pass parameters *via the child_spec*. While the original proposal still allows customization via start_link.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--

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 elixir-lang-core+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/8446b09b-9a7d-43ed-a1fc-22db3626a448%40Spark.

[Michał Muskała]

On 23 Feb 2017, 10:57 +0100, José Valim <jose....@plataformatec.com.br>, wrote:

Right. The issue also exists in the current proposal for any parameter that is passed via the child_spec. The trouble with your proposal is that the only option is to pass parameters *via the child_spec*. While the original proposal still allows customization via start_link.

What if we had a custom supervisor implementation that would always call child_spec before restarting?

[José Valim]

What if we had a custom supervisor implementation that would always call child_spec before restarting?

It is the opposite, if the supervisor restarts, then it reloads the child_spec. The problem is all other scenarios where any of the modules in your system may change but the supervisor did not (because it didn't have to). Examples of this include:

1. When compiling a Phoenix a project in development, we would need to traverse your application tree and ask all supervisors to reload childspecs

2. When building a relup, the release needs to go through all supervisors and tell them to reload childspecs

Another way to put it is that, in the same way our current worker/supervisor is broken because it moves the child_spec logic out of the supervisor, removing start_link will be broken because it moves the start_link logic inside the supervisor and outside of the supervised module.

[Michał Muskała]

I'm proposing that the supervisor *always* reloads the child spec before restarting a child. That way the problem goes away in both examples you showed.

Michał.

[José Valim]

Oh, I see. Let's see what James thinks about it although it comes with a very big concern of reimplementing the Supervisor behaviour.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--

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 elixir-lang-core+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/55932eea-667c-49d6-8de8-1cd263a61988%40Spark.

[Paul Schoenfelder]

Just my two cents, but if the change involves reimplementing the supervisor behaviour, I'm really not a fan - I have to say that neither of the proposals solves any problems I actually have, but that in and of itself isn't a reason to object, but re-implementing :supervisor seems like a huge change to make for something which (in my opinion) is almost entirely superficial.

José's proposal seems like the best of both worlds from my perspective, anything more invasive just makes the divide between Erlang and Elixir wider and more difficult to cross for many, and I still believe that the parity between the two languages is a strength, not a weakness. If there is a way to reduce boilerplate but still keep concepts trivially exportable between the two, then I'm all for it, but otherwise it just feels like the wrong direction to me.

Paul

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4JGLfSwKd%3DeSuMhW1MXJv_KhixfG6b88k6e%2B_K4SAWt8w%40mail.gmail.com.