[erlang-patches] allow use of proplists in supervisor start specs

[James Fish]

I think brutal_kill is a bad choice for supervisor children. When a
supervisor process exits it is very beneficial to know that (except for
very exceptional circumstances) every process below it in the supervisor
tree has exited. If these processes lower down the supervision have not
shutdown, they are now orphaned with no guarantee they will be
shutdown/exit.

The issue materialises when the child supervisor has a worker that traps
exits. The worker might be slow to terminate, perhaps because it has alot
of messages queued, makes a long blocking call or has got stuck in a call
that will never return due to a bug. When properly terminated by it's
parent, the worker might be killed or given a shutdown timeout but this
does not come into effect because its parent was killed, so the worker
remains alive for an undetermined period of time.

If/When the worker's parent is restarted a new version of the worker can
not be (re)started if the worker is registered because the old worker is
still alive. This could cause the supervisor that brutal kills it's child
supervisor to max out it's restart limit and continue to do so up the
supervision tree. This could have been avoided if the worker's parent had
been allowed to carry out its duty as a supervisor to terminate its
children.

Of course if the worker is not registered a new version can be restarted.
However in this case there could now be two versions of this worker when
the application was only designed to have one running at a time. This bug
probably never crops up in testing, perhaps because it was assumed the
worker's supervisor would prevent this case.

Note that everywhere above where I say the child is a worker, I could have
used the child type supervisor! This problem could go all the way down the
supervision tree.

I do not follow the view that different defaults for different types is too
complicated, it is one of these least complicated configuration options in
a supervisor spec and there are only two types.


On 1 July 2014 12:06, Loïc Hoguin <essen@REDACTED> wrote:

> Just wanted to say I agree with everything José said, except 3. the
> different defaults for different process types. It makes things too hard to
> keep track of.
>
> I think brutal_kill is a great choice because it makes it easier for the
> user to stumble upon cold-start issues when restarting their
> application/release. The VM may crash so there's never a guarantee to run
> shutdown functions anyway, and too few people forget to test cases when
> these didn't run, leading to potential production disasters.
>
>
> On 06/30/2014 04:37 PM, José Valim wrote:
>
>> I have some feedback based on my experience in writing and teaching
>> Erlang/Elixir.
>>
>> 1. I would always require the supervision strategy. When I am
>> prototyping, it is usually hard for me to reason about the intensity and
>> restarts, but I can always reason about the supervision strategy. All
>> teaching materials I know of also discuss the supervision right away, so
>> having it explicit shouldn't hurt those cases.
>>
>> For such cases, maybe we should use the format of {{strategy(), map()},
>> [children()]} (which is similar to what we have today).
>>
>> 2. I also agree with Richard. Having a default of 0 restarts may cause
>> confusion and forcing people to define them won't help when they still
>> have no idea of what values they should use (because it is prototyping /
>> early deploys). I usually fine tune those values based on the production
>> system reports so having saner defaults, like Ulf's, may make more sense.
>>
>> 3. The child_spec defaults are great and similar to the ones we use in
>> Elixir with the only difference being the shutdown value. We have a
>> default of 5000, which is the one usually recommended, but we set it to
>> "infinity" if the type is a "supervisor" (which is the OTP docs
>> recommendation if I remember correctly).
>>
>> Regarding 3., if the recommendation and the consensus in actual systems
>> is to use brutal_kill for shutdown, I will gladly change Elixir to make
>> it consistent with OTP defaults.
>>
>> Thanks for opening the changes for discussion!
>>
>> *José Valim*
>> www.plataformatec.com.br <http://www.plataformatec.com.br/>
>>
>> Skype: jv.ptec
>> Founder and Lead Developer
>>
>>
>>
>>
>> _______________________________________________
>> erlang-patches mailing list
>> erlang-patches@REDACTED
>> http://erlang.org/mailman/listinfo/erlang-patches
>>
>>
> --
> Loïc Hoguin
> http://ninenines.eu
>
> _______________________________________________
> erlang-patches mailing list
> erlang-patches@REDACTED
> http://erlang.org/mailman/listinfo/erlang-patches
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://erlang.org/pipermail/erlang-patches/attachments/20140701/31519f41/attachment.htm>