be more specific with die reasons in Supervisor

[tavisrudd]

Several calls to die were using opaque reasons that were unhelpful to
developers during debugging.

[hyperthunk]

Hmn, I wonder if we should die with an object instead of just a string here. What do you think? Do we ever want to actually catchExit and handle these?

[tavisrudd]

I'm not sure if it's worth it in this case. One is an internal error. The other is a failure to resolve the return value from the proc passed to toChildStart. That happens inside the starterProcess so there's no chance to catch it.

I'm not sure the die is sufficient here for the same reason. It would kill the starterProcess, which via a monitor eventually propagates out to the unfinished Left case in doRestartChild. The related childSpec would be removed. I think we need to at least log the failure cases in doRestartChild. Should the starterProcess be restarted?

[hyperthunk]

I think you're right - it's not worth changing.

I think we need to at least log the failure cases in doRestartChild.

Yes I agree with that. Notice or error level I think, probably error IMO.

Should the starterProcess be restarted?

I don't see how that's possible given the nature of the StarterProcess constructor - it just takes a pid and knows nothing about how the restarter process is/was implemented. I think we've simply got to treat that as a failure. Questions arise though:

• under what circumstances will this remove the child spec?
• depending on the restart type and policies in place, should this bring down the rest of the tree?
• do both those aspects actually work in code right now, or have we found a breakage/bug here?

[tavisrudd]

I think it should bring down the tree in the same cases where the child would be restarted: all but Temporary. Let's merge these die messages and I'll work on this in a separate PR. It might be best to include some extra error reporting mechanism for this case in your suggestion of spawnChildStarter on PR #77.

[hyperthunk]

Had to do this by hand due to merge conflicts. Thanks @tavisrudd - I'll get to the rest of the patches now.